From ea048b3b20d2bb2528de0ab975453f0496e73886 Mon Sep 17 00:00:00 2001 From: Sergiu Costea Date: Fri, 26 Apr 2019 15:40:58 +0200 Subject: [PATCH 1/3] Control plane clients can now perform SVC resolution If backwards compatibility is desired, speakers with SVC resolution support can be configured to fall back to using SVC addresses for data packets. --- go/lib/infra/infraenv/BUILD.bazel | 1 + go/lib/infra/infraenv/infraenv.go | 31 +- go/lib/infra/messenger/BUILD.bazel | 20 +- go/lib/infra/messenger/addr.go | 197 ++++++++++++ go/lib/infra/messenger/addr_test.go | 304 ++++++++++++++++++ go/lib/infra/messenger/messenger.go | 82 ++--- go/lib/infra/messenger/messenger_test.go | 75 ----- .../messenger/mock_messenger/BUILD.bazel | 14 + .../messenger/mock_messenger/messenger.go | 52 +++ go/lib/infra/modules/trust/BUILD.bazel | 1 + go/lib/infra/modules/trust/trust_test.go | 10 +- go/lib/snet/addr.go | 17 + go/lib/snet/dispatcher.go | 2 +- go/lib/snet/router.go | 24 ++ go/lib/svc/resolver.go | 41 +-- go/lib/svc/resolver_test.go | 29 +- tools/gomocks | 1 + 17 files changed, 723 insertions(+), 178 deletions(-) create mode 100644 go/lib/infra/messenger/addr.go create mode 100644 go/lib/infra/messenger/addr_test.go create mode 100644 go/lib/infra/messenger/mock_messenger/BUILD.bazel create mode 100644 go/lib/infra/messenger/mock_messenger/messenger.go diff --git a/go/lib/infra/infraenv/BUILD.bazel b/go/lib/infra/infraenv/BUILD.bazel index a5c5426d9b..ccc61c403c 100644 --- a/go/lib/infra/infraenv/BUILD.bazel +++ b/go/lib/infra/infraenv/BUILD.bazel @@ -19,5 +19,6 @@ go_library( "//go/lib/snet:go_default_library", "//go/lib/snet/snetproxy:go_default_library", "//go/lib/sock/reliable:go_default_library", + "//go/lib/svc:go_default_library", ], ) diff --git a/go/lib/infra/infraenv/infraenv.go b/go/lib/infra/infraenv/infraenv.go index 74797a1b9b..b8e15a38f3 100644 --- a/go/lib/infra/infraenv/infraenv.go +++ b/go/lib/infra/infraenv/infraenv.go @@ -34,6 +34,7 @@ import ( "github.com/scionproto/scion/go/lib/snet" "github.com/scionproto/scion/go/lib/snet/snetproxy" "github.com/scionproto/scion/go/lib/sock/reliable" + "github.com/scionproto/scion/go/lib/svc" ) const ( @@ -73,10 +74,27 @@ func (nc *NetworkConfig) Messenger() (infra.Messenger, error) { if err != nil { return nil, err } + + router := nc.Router + if router == nil { + router = &snet.BaseRouter{IA: nc.IA} + } + msgerCfg := &messenger.Config{ IA: nc.IA, TrustStore: nc.TrustStore, - Router: nc.Router, + AddressRewriter: &messenger.AddressRewriter{ + Router: router, + Resolver: &svc.Resolver{ + LocalIA: nc.IA, + ConnFactory: snet.NewDefaultPacketDispatcherService( + reliable.NewDispatcherService(""), + ), + Machine: buildLocalMachine(nc.Bind, nc.Public), + }, + // XXX(scrye): Disable SVC resolution for the moment. + SVCResolutionFraction: 0.00, + }, } if nc.EnableQUICTest { var err error @@ -97,6 +115,17 @@ func (nc *NetworkConfig) Messenger() (infra.Messenger, error) { } +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 +} + func (nc *NetworkConfig) initNetworking() (net.PacketConn, error) { var network snet.Network network, err := snet.NewNetwork(nc.IA, "", reliable.NewDispatcherService("")) diff --git a/go/lib/infra/messenger/BUILD.bazel b/go/lib/infra/messenger/BUILD.bazel index 876136c2e6..1b7ad01be3 100644 --- a/go/lib/infra/messenger/BUILD.bazel +++ b/go/lib/infra/messenger/BUILD.bazel @@ -4,6 +4,7 @@ go_library( name = "go_default_library", srcs = [ "adapter.go", + "addr.go", "counter.go", "messenger.go", "messenger_with_metrics.go", @@ -31,6 +32,7 @@ go_library( "//go/lib/prom:go_default_library", "//go/lib/scrypto:go_default_library", "//go/lib/snet:go_default_library", + "//go/lib/svc:go_default_library", "//go/lib/util:go_default_library", "//go/proto:go_default_library", "@com_github_lucas_clemente_quic_go//:go_default_library", @@ -40,18 +42,22 @@ go_library( go_test( name = "go_default_test", - srcs = ["messenger_test.go"], + srcs = [ + "addr_test.go", + "messenger_test.go", + ], embed = [":go_default_library"], deps = [ "//go/lib/addr:go_default_library", - "//go/lib/common:go_default_library", - "//go/lib/ctrl/cert_mgmt:go_default_library", - "//go/lib/infra:go_default_library", - "//go/lib/infra/disp:go_default_library", - "//go/lib/infra/transport:go_default_library", + "//go/lib/infra/messenger/mock_messenger:go_default_library", "//go/lib/log:go_default_library", + "//go/lib/overlay:go_default_library", + "//go/lib/snet:go_default_library", + "//go/lib/snet/mock_snet:go_default_library", + "//go/lib/spath:go_default_library", + "//go/lib/svc:go_default_library", "//go/lib/xtest:go_default_library", - "//go/lib/xtest/p2p:go_default_library", + "@com_github_golang_mock//gomock:go_default_library", "@com_github_smartystreets_goconvey//convey:go_default_library", ], ) diff --git a/go/lib/infra/messenger/addr.go b/go/lib/infra/messenger/addr.go new file mode 100644 index 0000000000..b5622e590c --- /dev/null +++ b/go/lib/infra/messenger/addr.go @@ -0,0 +1,197 @@ +// Copyright 2019 ETH Zurich +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package messenger + +import ( + "context" + "net" + "time" + + "github.com/scionproto/scion/go/lib/addr" + "github.com/scionproto/scion/go/lib/common" + "github.com/scionproto/scion/go/lib/log" + "github.com/scionproto/scion/go/lib/snet" + "github.com/scionproto/scion/go/lib/svc" +) + +// Resolver performs SVC resolution for a remote AS, thus converting an anycast +// SVC address to a unicast IP/UDP one. +type Resolver interface { + // LookupSVC resolves the SVC address for the AS terminating the path. + LookupSVC(ctx context.Context, path snet.Path, svc addr.HostSVC) (*svc.Reply, error) +} + +// AddressRewriter is used to compute paths and unicast addresses for SVC +// destinations. +type AddressRewriter struct { + // Router obtains path information to fill in address paths, if they are + // required and missing. + Router snet.Router + // Resolver performs SVC resolution if enabled. + Resolver Resolver + // SVCResolutionFraction enables SVC resolution for traffic to SVC + // destinations in a way that is also compatible with control plane servers + // that do not implement the SVC Resolution Mechanism. The value represents + // the percentage of time, out of the total available context timeout, + // spent attempting to perform SVC resolution. If SVCResolutionFraction is + // 0 or less, SVC resolution is never attempted. If it is between 0 and 1, + // the remaining context timeout is multiplied by the value, and that + // amount of time is spent waiting for an SVC resolution reply from the + // server. If this times out, the data packet is sent with an SVC + // destination. If the value is 1 or more, then legacy behavior is + // disabled, and data packets are never sent to SVC destinations unless the + // resolution step is successful. + SVCResolutionFraction float64 +} + +// Rewrite takes address a and adds a path (if one does not already exist but +// is required), and replaces SVC destinations with unicast ones, if desired. +func (r AddressRewriter) Rewrite(ctx context.Context, a net.Addr) (net.Addr, 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, nil + } + address, err := r.buildFullAddress(ctx, a) + if err != nil { + return nil, err + } + path, err := address.GetPath() + if err != nil { + return nil, common.NewBasicError("bad path", err) + } + address.Host, err = r.resolveIfSVC(ctx, path, address.Host) + return address, err + +} + +// buildFullAddress checks that a is a well-formed address (all fields set, +// 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) { + snetAddr, ok := a.(*snet.Addr) + if !ok { + return nil, common.NewBasicError("address type not supported", nil, "addr", a) + } + if snetAddr.Host == nil { + return nil, common.NewBasicError("host address not specified", nil, "addr", snetAddr) + } + if snetAddr.Host.L3 == nil { + return nil, common.NewBasicError("host address missing L3 address", nil, "addr", snetAddr) + } + if snetAddr.Host.L4 == nil { + return nil, common.NewBasicError("host address missing L4 address", nil, "addr", snetAddr) + } + if t := snetAddr.Host.L3.Type(); !addr.HostTypeCheck(t) { + return nil, common.NewBasicError("host address L3 address not supported", nil, "type", t) + } + if t := snetAddr.Host.L4.Type(); t != common.L4UDP { + return nil, common.NewBasicError("host address L4 address not supported", nil, "type", t) + } + newAddr := snetAddr.Copy() + + if newAddr.Path == nil { + p, err := r.Router.Route(ctx, newAddr.IA) + if err != nil { + return nil, err + } + newAddr.Path = p.Path() + newAddr.NextHop = p.OverlayNextHop() + } + return newAddr, nil +} + +// resolveIfSvc returns an UDP/IP address if the input address is an SVC +// destination. 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. +func (r AddressRewriter) resolveIfSVC(ctx context.Context, p snet.Path, + address *addr.AppAddr) (*addr.AppAddr, error) { + + svcAddress := getSVC(address.L3) + if svcAddress == addr.SvcNone { + return address.Copy(), nil + } + if r.SVCResolutionFraction <= 0.0 { + return address.Copy(), nil + } + + 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, + "svcResFraction", r.SVCResolutionFraction) + reply, err := r.Resolver.LookupSVC(ctx, p, svcAddress) + if err != nil { + 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 address.Copy(), 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) + return nil, err + } + logger.Trace("SVC resolution successful", "reply", reply) + return parseReply(reply) +} + +func (r AddressRewriter) resolutionCtx(ctx context.Context) (context.Context, context.CancelFunc) { + deadline, ok := ctx.Deadline() + if !ok { + return context.WithCancel(ctx) + } + + timeout := deadline.Sub(time.Now()) + timeout = time.Duration(float64(timeout) * r.SVCResolutionFraction) + return context.WithTimeout(ctx, timeout) +} + +func getSVC(a addr.HostAddr) addr.HostSVC { + if svcAddress, ok := a.(addr.HostSVC); ok { + return svcAddress + } + return addr.SvcNone +} + +// parseReply searches for a UDP server on the remote address. If one is not +// found, an error is returned. +func parseReply(reply *svc.Reply) (*addr.AppAddr, error) { + if reply == nil { + return nil, common.NewBasicError("nil reply", nil) + } + if reply.Transports == nil { + return nil, common.NewBasicError("empty reply", nil) + } + addressStr, ok := reply.Transports[svc.UDP] + if !ok { + return nil, common.NewBasicError("UDP server address not found", nil) + } + 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: addr.NewL4UDPInfo(uint16(udpAddr.Port)), + }, nil +} diff --git a/go/lib/infra/messenger/addr_test.go b/go/lib/infra/messenger/addr_test.go new file mode 100644 index 0000000000..a5807b9c16 --- /dev/null +++ b/go/lib/infra/messenger/addr_test.go @@ -0,0 +1,304 @@ +// Copyright 2019 ETH Zurich +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package messenger + +import ( + "context" + "fmt" + "net" + "testing" + + "github.com/golang/mock/gomock" + . "github.com/smartystreets/goconvey/convey" + + "github.com/scionproto/scion/go/lib/addr" + "github.com/scionproto/scion/go/lib/infra/messenger/mock_messenger" + "github.com/scionproto/scion/go/lib/overlay" + "github.com/scionproto/scion/go/lib/snet" + "github.com/scionproto/scion/go/lib/snet/mock_snet" + "github.com/scionproto/scion/go/lib/spath" + "github.com/scionproto/scion/go/lib/svc" + "github.com/scionproto/scion/go/lib/xtest" +) + +func TestBuildFullAddress(t *testing.T) { + testCases := []struct { + Description string + InputAddress net.Addr + ExpectedAddress *snet.Addr + ExpectedError bool + }{ + { + Description: "non-snet address", + InputAddress: &net.UDPAddr{}, + ExpectedError: true, + }, + { + Description: "snet address without host", + InputAddress: &snet.Addr{}, + ExpectedError: true, + }, + { + Description: "snet address without L3", + InputAddress: &snet.Addr{Host: &addr.AppAddr{}}, + ExpectedError: true, + }, + { + Description: "snet address without L4", + InputAddress: &snet.Addr{Host: &addr.AppAddr{L3: addr.SvcBS}}, + ExpectedError: true, + }, + { + Description: "snet address with bad L3 type", + InputAddress: &snet.Addr{ + Host: &addr.AppAddr{ + L3: &addr.HostNone{}, + L4: addr.NewL4UDPInfo(5), + }, + }, + ExpectedError: true, + }, + { + Description: "snet address with bad L4 type", + InputAddress: &snet.Addr{ + Host: &addr.AppAddr{ + L3: addr.SvcBS, + L4: addr.NewL4TCPInfo(5), + }, + }, + ExpectedError: true, + }, + } + Convey("", t, func() { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + router := mock_snet.NewMockRouter(ctrl) + resolver := mock_messenger.NewMockResolver(ctrl) + aw := AddressRewriter{ + Resolver: resolver, + Router: router, + } + for _, tc := range testCases { + Convey(tc.Description, func() { + a, err := aw.buildFullAddress(context.Background(), tc.InputAddress) + SoMsg("addr", a, ShouldResemble, tc.ExpectedAddress) + xtest.SoMsgError("err", err, tc.ExpectedError) + }) + } + }) + Convey("", t, func() { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + router := mock_snet.NewMockRouter(ctrl) + aw := AddressRewriter{ + Router: router, + } + Convey("snet address without path, error retrieving path", func() { + inputAddress := &snet.Addr{ + Host: &addr.AppAddr{ + L3: addr.SvcBS, + L4: addr.NewL4UDPInfo(1), + }, + } + router.EXPECT().Route(gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("err")) + _, err := aw.buildFullAddress(context.Background(), inputAddress) + SoMsg("err", err, ShouldNotBeNil) + }) + Convey("snet address with path", func() { + inputAddress := &snet.Addr{ + Host: &addr.AppAddr{ + L3: addr.SvcBS, + L4: addr.NewL4UDPInfo(1), + }, + Path: &spath.Path{}, + } + a, err := aw.buildFullAddress(context.Background(), inputAddress) + SoMsg("addr", a, ShouldResemble, inputAddress) + SoMsg("err", err, ShouldBeNil) + }) + Convey("snet address without path, successful retrieving path", func() { + path := mock_snet.NewMockPath(ctrl) + path.EXPECT().Path().Return(&spath.Path{}) + path.EXPECT().OverlayNextHop().Return(&overlay.OverlayAddr{}) + router.EXPECT().Route(gomock.Any(), gomock.Any()).Return(path, nil) + inputAddress := &snet.Addr{ + Host: &addr.AppAddr{ + L3: addr.SvcBS, + L4: addr.NewL4UDPInfo(1), + }, + } + a, err := aw.buildFullAddress(context.Background(), inputAddress) + SoMsg("addr", a, ShouldResemble, &snet.Addr{ + Host: &addr.AppAddr{ + L3: addr.SvcBS, + L4: addr.NewL4UDPInfo(1), + }, + Path: &spath.Path{}, + NextHop: &overlay.OverlayAddr{}, + }) + SoMsg("err", err, ShouldBeNil) + }) + }) +} + +func TestResolveIfSVC(t *testing.T) { + testCases := []struct { + Description string + InputAddress *addr.AppAddr + ResolverSetup func(*mock_messenger.MockResolver) + ExpectedAddress *addr.AppAddr + ExpectedError bool + }{ + { + Description: "non-svc address does not trigger lookup", + InputAddress: &addr.AppAddr{ + L3: addr.HostFromIP(net.IP{192, 168, 0, 1}), + L4: addr.NewL4UDPInfo(1), + }, + ExpectedAddress: &addr.AppAddr{ + L3: addr.HostFromIP(net.IP{192, 168, 0, 1}), + L4: addr.NewL4UDPInfo(1), + }, + }, + { + Description: "svc address, lookup fails", + InputAddress: &addr.AppAddr{ + L3: addr.SvcBS, + L4: addr.NewL4UDPInfo(1), + }, + ResolverSetup: func(r *mock_messenger.MockResolver) { + r.EXPECT().LookupSVC(gomock.Any(), gomock.Any(), gomock.Any()). + Return(nil, fmt.Errorf("err")) + }, + ExpectedError: true, + }, + { + Description: "svc address, lookup succeeds", + InputAddress: &addr.AppAddr{ + L3: addr.SvcBS, + L4: addr.NewL4UDPInfo(1), + }, + ResolverSetup: func(r *mock_messenger.MockResolver) { + r.EXPECT().LookupSVC(gomock.Any(), gomock.Any(), gomock.Any()). + Return( + &svc.Reply{ + Transports: map[svc.Transport]string{ + svc.UDP: "192.168.1.1:8000", + }, + }, + nil, + ) + }, + ExpectedAddress: &addr.AppAddr{ + L3: addr.HostFromIP(net.IP{192, 168, 1, 1}), + L4: addr.NewL4UDPInfo(8000), + }, + }, + } + + Convey("", t, func() { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + resolver := mock_messenger.NewMockResolver(ctrl) + path := mock_snet.NewMockPath(ctrl) + path.EXPECT().Destination().Return(addr.IA{}).AnyTimes() + aw := AddressRewriter{ + Resolver: resolver, + SVCResolutionFraction: 1.0, + } + for _, tc := range testCases { + Convey(tc.Description, func() { + initResolver(resolver, tc.ResolverSetup) + a, err := aw.resolveIfSVC(context.Background(), path, tc.InputAddress) + SoMsg("addr", a, ShouldResemble, tc.ExpectedAddress) + xtest.SoMsgError("err", err, tc.ExpectedError) + }) + } + }) +} + +func TestParseReply(t *testing.T) { + Convey("", t, func() { + testCases := []struct { + Description string + Reply *svc.Reply + ExpectedAddress *addr.AppAddr + ExpectedError bool + }{ + { + Description: "nil reply", + ExpectedError: true, + }, + { + Description: "empty reply", + Reply: &svc.Reply{}, + ExpectedError: true, + }, + { + Description: "key not found in reply", + Reply: &svc.Reply{Transports: map[svc.Transport]string{svc.QUIC: "foo"}}, + ExpectedError: true, + }, + { + Description: "key found in reply, but parsing fails", + Reply: &svc.Reply{ + Transports: map[svc.Transport]string{ + svc.UDP: "foo", + }, + }, + ExpectedError: true, + }, + { + Description: "key found in reply, IPv4 address", + Reply: &svc.Reply{ + Transports: map[svc.Transport]string{ + svc.UDP: "192.168.1.1:8000", + }, + }, + ExpectedAddress: &addr.AppAddr{ + L3: addr.HostFromIP(net.IP{192, 168, 1, 1}), + L4: addr.NewL4UDPInfo(8000), + }, + ExpectedError: false, + }, + { + Description: "key found in reply, IPv6 address", + Reply: &svc.Reply{ + Transports: map[svc.Transport]string{ + svc.UDP: "[2001:db8::1]:8000", + }, + }, + ExpectedAddress: &addr.AppAddr{ + L3: addr.HostFromIP(net.ParseIP("2001:db8::1")), + L4: addr.NewL4UDPInfo(8000), + }, + ExpectedError: false, + }, + } + for _, tc := range testCases { + Convey(tc.Description, func() { + a, err := parseReply(tc.Reply) + xtest.SoMsgError("err", err, tc.ExpectedError) + SoMsg("addr", a, ShouldResemble, tc.ExpectedAddress) + }) + } + }) +} + +func initResolver(resolver *mock_messenger.MockResolver, f func(*mock_messenger.MockResolver)) { + if f != nil { + f(resolver) + } +} diff --git a/go/lib/infra/messenger/messenger.go b/go/lib/infra/messenger/messenger.go index 1a79896e8a..e4b37cb02a 100644 --- a/go/lib/infra/messenger/messenger.go +++ b/go/lib/infra/messenger/messenger.go @@ -107,9 +107,9 @@ type Config struct { Dispatcher *disp.Dispatcher // TrustStore stores and retrieves certificate chains and TRCs. TrustStore infra.TrustStore - // Router is used to discover paths to remote ASes. If router is nil, the - // messenger never resolves paths. - Router snet.Router + // AddressRewriter is used to compute paths and unicast addresses for SVC + // destinations. AddressRewriter must not be nil. + AddressRewriter *AddressRewriter // HandlerTimeout is the amount of time allocated to the processing of a // received message. This includes the time needed to verify the signature // and the execution of a registered handler (if one exists). If the @@ -153,8 +153,9 @@ type Messenger struct { config *Config // Networking layer for sending and receiving messages dispatcher *disp.Dispatcher - // router is used to discover paths to remote ASes - router snet.Router + + // addressRewriter is used to compute full remote addresses (path + server) + addressRewriter *AddressRewriter cryptoLock sync.RWMutex // signer is used to sign selected outgoing messages @@ -206,10 +207,6 @@ func New(config *Config) *Messenger { } } - router := config.Router - if router == nil { - router = &snet.BaseRouter{IA: config.IA} - } // XXX(scrye): A trustStore object is passed to the Messenger as it is required // to verify top-level signatures. This is never used right now since only // unsigned messages are supported. The content of received messages is @@ -217,20 +214,20 @@ func New(config *Config) *Messenger { // trustStore. ctx, cancelF := context.WithCancel(context.Background()) return &Messenger{ - ia: config.IA, - config: config, - dispatcher: config.Dispatcher, - router: router, - signer: infra.NullSigner, - verifier: infra.NullSigVerifier, - trustStore: config.TrustStore, - handlers: make(map[infra.MessageType]infra.Handler), - closeChan: make(chan struct{}), - ctx: ctx, - cancelF: cancelF, - log: config.Logger, - quicServer: quicServer, - quicHandler: quicHandler, + ia: config.IA, + config: config, + dispatcher: config.Dispatcher, + addressRewriter: config.AddressRewriter, + signer: infra.NullSigner, + verifier: infra.NullSigVerifier, + trustStore: config.TrustStore, + handlers: make(map[infra.MessageType]infra.Handler), + closeChan: make(chan struct{}), + ctx: ctx, + cancelF: cancelF, + log: config.Logger, + quicServer: quicServer, + quicHandler: quicHandler, } } @@ -764,8 +761,8 @@ func (m *Messenger) getRequester(reqT infra.MessageType) Requester { } if m.config.QUIC == nil { return &pathingRequester{ - requester: ctrl_msg.NewRequester(signer, m.verifier, m.dispatcher), - router: m.router, + requester: ctrl_msg.NewRequester(signer, m.verifier, m.dispatcher), + addressRewriter: m.addressRewriter, } } return &QUICRequester{ @@ -774,7 +771,7 @@ func (m *Messenger) getRequester(reqT infra.MessageType) Requester { TLSConfig: m.config.QUIC.TLSConfig, QUICConfig: m.config.QUIC.QUICConfig, }, - Router: m.router, + AddressRewriter: m.addressRewriter, } } @@ -794,14 +791,14 @@ var _ Requester = (*pathingRequester)(nil) // pathingRequester resolves the SCION path and constructs complete snet // addresses. type pathingRequester struct { - requester *ctrl_msg.Requester - router snet.Router + requester *ctrl_msg.Requester + addressRewriter *AddressRewriter } func (pr *pathingRequester) Request(ctx context.Context, pld *ctrl.Pld, a net.Addr) (*ctrl.Pld, *proto.SignS, error) { - newAddr, err := computeNewAddress(ctx, pr.router, a) + newAddr, err := pr.addressRewriter.Rewrite(ctx, a) if err != nil { return nil, nil, err } @@ -809,7 +806,7 @@ func (pr *pathingRequester) Request(ctx context.Context, pld *ctrl.Pld, } func (pr *pathingRequester) Notify(ctx context.Context, pld *ctrl.Pld, a net.Addr) error { - newAddr, err := computeNewAddress(ctx, pr.router, a) + newAddr, err := pr.addressRewriter.Rewrite(ctx, a) if err != nil { return err } @@ -817,7 +814,7 @@ func (pr *pathingRequester) Notify(ctx context.Context, pld *ctrl.Pld, a net.Add } func (pr *pathingRequester) NotifyUnreliable(ctx context.Context, pld *ctrl.Pld, a net.Addr) error { - newAddr, err := computeNewAddress(ctx, pr.router, a) + newAddr, err := pr.addressRewriter.Rewrite(ctx, a) if err != nil { return err } @@ -828,7 +825,7 @@ var _ Requester = (*QUICRequester)(nil) type QUICRequester struct { QUICClientConfig *rpc.Client - Router snet.Router + AddressRewriter *AddressRewriter } func (rt *QUICRequester) Request(ctx context.Context, pld *ctrl.Pld, @@ -836,7 +833,7 @@ func (rt *QUICRequester) Request(ctx context.Context, pld *ctrl.Pld, // FIXME(scrye): Rely on QUIC for security for now. This needs to do // additional verifications in the future. - newAddr, err := computeNewAddress(ctx, rt.Router, a) + newAddr, err := rt.AddressRewriter.Rewrite(ctx, a) if err != nil { return nil, nil, err } @@ -867,25 +864,6 @@ func (rt *QUICRequester) NotifyUnreliable(ctx context.Context, pld *ctrl.Pld, a return common.NewBasicError("transport type does not support NotifyUnreliable message", nil) } -func computeNewAddress(ctx context.Context, r snet.Router, a net.Addr) (net.Addr, error) { - // FIXME(scrye): This is here just to make the trust store pass some older - // tests that use a full messenger instead of mocks. - // See https://github.com/scionproto/scion/issues/2611. - if a == nil { - return nil, nil - } - newAddr := a.(*snet.Addr).Copy() - if newAddr.Path == nil { - p, err := r.Route(ctx, newAddr.IA) - if err != nil { - return nil, err - } - newAddr.Path = p.Path() - newAddr.NextHop = p.OverlayNextHop() - } - return newAddr, nil -} - type Request struct { Host net.Addr Payload *ctrl.SignedPld diff --git a/go/lib/infra/messenger/messenger_test.go b/go/lib/infra/messenger/messenger_test.go index 5a77a631d9..bc204c0a1b 100644 --- a/go/lib/infra/messenger/messenger_test.go +++ b/go/lib/infra/messenger/messenger_test.go @@ -15,87 +15,12 @@ package messenger import ( - "context" - "net" "os" "testing" - "time" - . "github.com/smartystreets/goconvey/convey" - - "github.com/scionproto/scion/go/lib/addr" - "github.com/scionproto/scion/go/lib/common" - "github.com/scionproto/scion/go/lib/ctrl/cert_mgmt" - "github.com/scionproto/scion/go/lib/infra" - "github.com/scionproto/scion/go/lib/infra/disp" - "github.com/scionproto/scion/go/lib/infra/transport" "github.com/scionproto/scion/go/lib/log" - "github.com/scionproto/scion/go/lib/xtest" - "github.com/scionproto/scion/go/lib/xtest/p2p" -) - -// TestCase data -var ( - mockTRC = &cert_mgmt.TRC{RawTRC: common.RawBytes("foobar")} ) -func MockTRCHandler(request *infra.Request) *infra.HandlerResult { - rw, ok := infra.ResponseWriterFromContext(request.Context()) - if !ok { - log.Warn("Unable to service request, no resopnse writer found") - return infra.MetricsErrInternal - } - subCtx, cancelF := context.WithTimeout(request.Context(), 3*time.Second) - defer cancelF() - if err := rw.SendTRCReply(subCtx, mockTRC); err != nil { - log.Error("Server error", "err", err) - return infra.MetricsErrInternal - } - return infra.MetricsResultOk -} - -func TestTRCExchange(t *testing.T) { - Convey("Setup", t, func() { - c2s, s2c := p2p.NewPacketConns() - clientMessenger := setupMessenger(xtest.MustParseIA("1-ff00:0:1"), c2s, "client") - serverMessenger := setupMessenger(xtest.MustParseIA("2-ff00:0:1"), s2c, "server") - - Convey("Client/server", xtest.Parallel(func(sc *xtest.SC) { - // The client sends a TRC request to the server, and receives the - // TRC. - ctx, cancelF := context.WithTimeout(context.Background(), 3*time.Second) - defer cancelF() - - msg := &cert_mgmt.TRCReq{ISD: 42, Version: 1337, CacheOnly: true} - trc, err := clientMessenger.GetTRC(ctx, msg, nil, 1337) - // CloseServer now, to guarantee it is run even if an assertion - // fails and execution of the client stops - serverMessenger.CloseServer() - sc.SoMsg("client request err", err, ShouldBeNil) - sc.SoMsg("client received trc", trc, ShouldResemble, mockTRC) - }, func(sc *xtest.SC) { - // The server receives a TRC request from the client, passes it to - // the mock TRCRequest handler which sends back the result. - serverMessenger.AddHandler(infra.TRCRequest, infra.HandlerFunc(MockTRCHandler)) - serverMessenger.ListenAndServe() - })) - }) -} - -func setupMessenger(ia addr.IA, conn net.PacketConn, name string) *Messenger { - config := &Config{ - IA: ia, - DisableSignatureVerification: true, - Dispatcher: disp.New( - transport.NewPacketTransport(conn), - DefaultAdapter, - log.New("name", name), - ), - Logger: log.Root().New("name", name), - } - return New(config) -} - func TestMain(m *testing.M) { log.Root().SetHandler(log.DiscardHandler()) os.Exit(m.Run()) diff --git a/go/lib/infra/messenger/mock_messenger/BUILD.bazel b/go/lib/infra/messenger/mock_messenger/BUILD.bazel new file mode 100644 index 0000000000..67c7d19a02 --- /dev/null +++ b/go/lib/infra/messenger/mock_messenger/BUILD.bazel @@ -0,0 +1,14 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_library") + +go_library( + name = "go_default_library", + srcs = ["messenger.go"], + importpath = "github.com/scionproto/scion/go/lib/infra/messenger/mock_messenger", + visibility = ["//visibility:public"], + deps = [ + "//go/lib/addr:go_default_library", + "//go/lib/snet:go_default_library", + "//go/lib/svc:go_default_library", + "@com_github_golang_mock//gomock:go_default_library", + ], +) diff --git a/go/lib/infra/messenger/mock_messenger/messenger.go b/go/lib/infra/messenger/mock_messenger/messenger.go new file mode 100644 index 0000000000..42eb34d15a --- /dev/null +++ b/go/lib/infra/messenger/mock_messenger/messenger.go @@ -0,0 +1,52 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/scionproto/scion/go/lib/infra/messenger (interfaces: Resolver) + +// Package mock_messenger is a generated GoMock package. +package mock_messenger + +import ( + context "context" + gomock "github.com/golang/mock/gomock" + addr "github.com/scionproto/scion/go/lib/addr" + snet "github.com/scionproto/scion/go/lib/snet" + svc "github.com/scionproto/scion/go/lib/svc" + reflect "reflect" +) + +// MockResolver is a mock of Resolver interface +type MockResolver struct { + ctrl *gomock.Controller + recorder *MockResolverMockRecorder +} + +// MockResolverMockRecorder is the mock recorder for MockResolver +type MockResolverMockRecorder struct { + mock *MockResolver +} + +// NewMockResolver creates a new mock instance +func NewMockResolver(ctrl *gomock.Controller) *MockResolver { + mock := &MockResolver{ctrl: ctrl} + mock.recorder = &MockResolverMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use +func (m *MockResolver) EXPECT() *MockResolverMockRecorder { + return m.recorder +} + +// LookupSVC mocks base method +func (m *MockResolver) LookupSVC(arg0 context.Context, arg1 snet.Path, arg2 addr.HostSVC) (*svc.Reply, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "LookupSVC", arg0, arg1, arg2) + ret0, _ := ret[0].(*svc.Reply) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// LookupSVC indicates an expected call of LookupSVC +func (mr *MockResolverMockRecorder) LookupSVC(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "LookupSVC", reflect.TypeOf((*MockResolver)(nil).LookupSVC), arg0, arg1, arg2) +} diff --git a/go/lib/infra/modules/trust/BUILD.bazel b/go/lib/infra/modules/trust/BUILD.bazel index e6b567b4f1..8cbc4045a8 100644 --- a/go/lib/infra/modules/trust/BUILD.bazel +++ b/go/lib/infra/modules/trust/BUILD.bazel @@ -56,6 +56,7 @@ go_test( "//go/lib/scrypto:go_default_library", "//go/lib/scrypto/cert:go_default_library", "//go/lib/scrypto/trc:go_default_library", + "//go/lib/snet:go_default_library", "//go/lib/topology:go_default_library", "//go/lib/topology/topotestutil:go_default_library", "//go/lib/xtest:go_default_library", diff --git a/go/lib/infra/modules/trust/trust_test.go b/go/lib/infra/modules/trust/trust_test.go index dd731d4883..016165af66 100644 --- a/go/lib/infra/modules/trust/trust_test.go +++ b/go/lib/infra/modules/trust/trust_test.go @@ -41,6 +41,7 @@ import ( "github.com/scionproto/scion/go/lib/scrypto" "github.com/scionproto/scion/go/lib/scrypto/cert" "github.com/scionproto/scion/go/lib/scrypto/trc" + "github.com/scionproto/scion/go/lib/snet" "github.com/scionproto/scion/go/lib/topology" "github.com/scionproto/scion/go/lib/topology/topotestutil" "github.com/scionproto/scion/go/lib/xtest" @@ -256,7 +257,7 @@ func TestGetTRC(t *testing.T) { insertTRC(t, store, trcs[1]) insertTRC(t, store, trcs[3]) - for _, tc := range testCases[4:5] { + for _, tc := range testCases { Convey(tc.Name, func() { ctx, cancelF := context.WithTimeout(context.Background(), testCtxTimeout) defer cancelF() @@ -309,7 +310,7 @@ func TestGetValidChain(t *testing.T) { store, cleanF := initStore(t, ctrl, xtest.MustParseIA("1-ff00:0:1"), msger) defer cleanF() insertTRC(t, store, trcs[1]) - for _, tc := range testCases[3:4] { + for _, tc := range testCases { Convey(tc.Name, func() { ctx, cancelF := context.WithTimeout(context.Background(), testCtxTimeout) defer cancelF() @@ -697,6 +698,11 @@ func setupMessenger(ia addr.IA, conn net.PacketConn, store *Store, name string) messenger.DefaultAdapter, log.New("name", name), ), + AddressRewriter: &messenger.AddressRewriter{ + Router: &snet.BaseRouter{ + IA: ia, + }, + }, TrustStore: store, DisableSignatureVerification: true, Logger: log.Root().New("name", name), diff --git a/go/lib/snet/addr.go b/go/lib/snet/addr.go index bdf6ab0ad0..c796d43eb9 100644 --- a/go/lib/snet/addr.go +++ b/go/lib/snet/addr.go @@ -44,6 +44,23 @@ func (a *Addr) Network() string { return "scion" } +// GetPath returns a path with attached metadata. +func (a *Addr) GetPath() (Path, error) { + // Initialize path so it is always ready for use + var p *spath.Path + if a.Path != nil { + p = a.Path.Copy() + if err := p.InitOffsets(); err != nil { + return nil, err + } + } + return &partialPath{ + spath: p, + overlay: a.NextHop, + destination: a.IA, + }, nil +} + func (a *Addr) String() string { if a == nil { return "" diff --git a/go/lib/snet/dispatcher.go b/go/lib/snet/dispatcher.go index c7582dbc1c..eaf216d355 100644 --- a/go/lib/snet/dispatcher.go +++ b/go/lib/snet/dispatcher.go @@ -47,7 +47,7 @@ func (s *DefaultPacketDispatcherService) RegisterTimeout(ia addr.IA, public *add bind *overlay.OverlayAddr, svc addr.HostSVC, timeout time.Duration) (PacketConn, uint16, error) { - rconn, port, err := s.dispatcherService.Register(ia, public, bind, svc) + rconn, port, err := s.dispatcherService.RegisterTimeout(ia, public, bind, svc, timeout) if err != nil { return nil, 0, err } diff --git a/go/lib/snet/router.go b/go/lib/snet/router.go index 96e9b52b22..3b821bd7b8 100644 --- a/go/lib/snet/router.go +++ b/go/lib/snet/router.go @@ -136,6 +136,30 @@ func (p *path) Destination() addr.IA { return p.sciondPath.Path.DstIA() } +// partialPath is a path object with incomplete metadata. It is used as a +// temporary solution where a full path cannot be reconstituted from other +// objects, notably snet.Addr. +type partialPath struct { + spath *spath.Path + overlay *overlay.OverlayAddr + destination addr.IA +} + +func (p *partialPath) OverlayNextHop() *overlay.OverlayAddr { + return p.overlay +} + +func (p *partialPath) Path() *spath.Path { + if p.spath == nil { + return nil + } + return p.spath.Copy() +} + +func (p *partialPath) Destination() addr.IA { + return p.destination +} + // LocalMachine describes aspects of the host system and its network. type LocalMachine struct { // InterfaceIP is the default L3 address for connections originating from diff --git a/go/lib/svc/resolver.go b/go/lib/svc/resolver.go index 7790955fd5..d8578cb125 100644 --- a/go/lib/svc/resolver.go +++ b/go/lib/svc/resolver.go @@ -32,12 +32,16 @@ const ( errNilPacket = "packet is nil" errNilOverlay = "overlay is nil" errUnsupportedPld = "unsupported payload type" + errRegistration = "unable to open conn" + errWrite = "unable to write" + errRead = "unable to read" + errDecode = "decode failed" ) // Resolver performs SVC address resolution. type Resolver struct { - // Router is used to compute paths to remote ASes. - Router snet.Router + // LocalIA is the local AS. + LocalIA addr.IA // ConnFactory is used to open ports for SVC resolution messages. ConnFactory snet.PacketDispatcherService // Machine is used to derive addressing information for local conns. @@ -47,37 +51,36 @@ type Resolver struct { RoundTripper RoundTripper } -// LookupSVC resolves SVC addresses for a remote AS. -func (r *Resolver) LookupSVC(ctx context.Context, ia addr.IA, svc addr.HostSVC) (*Reply, error) { - path, err := r.Router.Route(ctx, ia) +// LookupSVC resolves the SVC address for the AS terminating the path. +func (r *Resolver) LookupSVC(ctx context.Context, p snet.Path, svc addr.HostSVC) (*Reply, error) { + // FIXME(scrye): Assume registration is always instant for now. This, + // however, should respect ctx. + conn, port, err := r.ConnFactory.RegisterTimeout(r.LocalIA, r.Machine.AppAddress(), + nil, addr.SvcNone, 0) if err != nil { - return nil, err - } - - conn, port, err := r.ConnFactory.RegisterTimeout(ia, r.Machine.AppAddress(), - r.Machine.BindAddress(), addr.SvcNone, 0) - if err != nil { - return nil, err + return nil, common.NewBasicError(errRegistration, err) } defer conn.Close() requestPacket := &snet.SCIONPacket{ SCIONPacketInfo: snet.SCIONPacketInfo{ Source: snet.SCIONAddress{ - IA: r.Router.LocalIA(), + IA: r.LocalIA, Host: r.Machine.AppAddress().L3, }, Destination: snet.SCIONAddress{ - IA: ia, + IA: p.Destination(), Host: svc, }, - Path: path.Path(), + Path: p.Path(), L4Header: &l4.UDP{ SrcPort: port, }, + // FIXME(scrye): Add a dummy payload, because nil payloads are not supported. + Payload: common.RawBytes{0}, }, } - return r.getRoundTripper().RoundTrip(ctx, conn, requestPacket, path.OverlayNextHop()) + return r.getRoundTripper().RoundTrip(ctx, conn, requestPacket, p.OverlayNextHop()) } func (r *Resolver) getRoundTripper() RoundTripper { @@ -119,13 +122,13 @@ func (roundTripper) RoundTrip(ctx context.Context, c snet.PacketConn, pkt *snet. defer cancelF() if err := c.WriteTo(pkt, ov); err != nil { - return nil, err + return nil, common.NewBasicError(errWrite, err) } var replyPacket snet.SCIONPacket var replyOv overlay.OverlayAddr if err := c.ReadFrom(&replyPacket, &replyOv); err != nil { - return nil, err + return nil, common.NewBasicError(errRead, err) } b, ok := replyPacket.Payload.(common.RawBytes) if !ok { @@ -133,7 +136,7 @@ func (roundTripper) RoundTrip(ctx context.Context, c snet.PacketConn, pkt *snet. } var reply Reply if err := reply.DecodeFrom(bytes.NewBuffer([]byte(b))); err != nil { - return nil, err + return nil, common.NewBasicError(errDecode, err) } return &reply, nil } diff --git a/go/lib/svc/resolver_test.go b/go/lib/svc/resolver_test.go index b7ac258668..25cb955be3 100644 --- a/go/lib/svc/resolver_test.go +++ b/go/lib/svc/resolver_test.go @@ -43,46 +43,33 @@ func TestResolver(t *testing.T) { srcIA := xtest.MustParseIA("1-ff00:0:1") dstIA := xtest.MustParseIA("1-ff00:0:2") - mockRouter := mock_snet.NewMockRouter(ctrl) - mockRouter.EXPECT().LocalIA().Return(srcIA).AnyTimes() mockPath := mock_snet.NewMockPath(ctrl) mockPath.EXPECT().Path().Return(nil).AnyTimes() mockPath.EXPECT().OverlayNextHop().Return(nil).AnyTimes() + mockPath.EXPECT().Destination().Return(dstIA).AnyTimes() - Convey("If routing fails, return error and no reply", func() { - mockRouter.EXPECT().Route(gomock.Any(), dstIA).Return(nil, errors.New("no path")) - resolver := &svc.Resolver{ - Router: mockRouter, - } - - reply, err := resolver.LookupSVC(context.Background(), dstIA, addr.SvcCS) - SoMsg("reply", reply, ShouldBeNil) - SoMsg("err", err, ShouldNotBeNil) - }) - Convey("If routing succeeds, but opening up port fails, return error and no reply", func() { + Convey("If opening up port fails, return error and no reply", func() { mockPacketDispatcherService := mock_snet.NewMockPacketDispatcherService(ctrl) - mockRouter.EXPECT().Route(gomock.Any(), dstIA).Return(mockPath, nil) mockPacketDispatcherService.EXPECT().RegisterTimeout(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). Return(nil, uint16(0), errors.New("no conn")) resolver := &svc.Resolver{ - Router: mockRouter, + LocalIA: srcIA, ConnFactory: mockPacketDispatcherService, } - reply, err := resolver.LookupSVC(context.Background(), dstIA, addr.SvcCS) + reply, err := resolver.LookupSVC(context.Background(), mockPath, addr.SvcCS) SoMsg("reply", reply, ShouldBeNil) SoMsg("err", err, ShouldNotBeNil) }) Convey("Local machine information is used to build conns", func() { machine := snet.LocalMachine{InterfaceIP: net.IP{192, 0, 2, 1}} - mockRouter.EXPECT().Route(gomock.Any(), dstIA).Return(mockPath, nil) mockPacketDispatcherService := mock_snet.NewMockPacketDispatcherService(ctrl) mockConn := mock_snet.NewMockPacketConn(ctrl) mockConn.EXPECT().Close() - mockPacketDispatcherService.EXPECT().RegisterTimeout(dstIA, + mockPacketDispatcherService.EXPECT().RegisterTimeout(srcIA, machine.AppAddress(), - machine.BindAddress(), + nil, addr.SvcNone, time.Duration(0)).Return(mockConn, uint16(42), nil) mockRoundTripper := mock_svc.NewMockRoundTripper(ctrl) @@ -90,12 +77,12 @@ func TestResolver(t *testing.T) { gomock.Any()) resolver := &svc.Resolver{ - Router: mockRouter, + LocalIA: srcIA, ConnFactory: mockPacketDispatcherService, Machine: machine, RoundTripper: mockRoundTripper, } - resolver.LookupSVC(context.Background(), dstIA, addr.SvcCS) + resolver.LookupSVC(context.Background(), mockPath, addr.SvcCS) }) }) } diff --git a/tools/gomocks b/tools/gomocks index 0d143320c1..2871c0614b 100755 --- a/tools/gomocks +++ b/tools/gomocks @@ -37,6 +37,7 @@ MOCK_TARGETS = [ "IfStatePusher,Beaconer,RevDropper"), (SCION_PACKAGE_PREFIX + "/go/lib/ctrl/seg", "Signer"), (SCION_PACKAGE_PREFIX + "/go/lib/infra", "TrustStore,Messenger,ResponseWriter,Verifier"), + (SCION_PACKAGE_PREFIX + "/go/lib/infra/messenger", "Resolver"), (SCION_PACKAGE_PREFIX + "/go/lib/infra/modules/trust/trustdb", "TrustDB"), (SCION_PACKAGE_PREFIX + "/go/lib/l4", "L4Header"), (SCION_PACKAGE_PREFIX + "/go/lib/log", "Handler,Logger"), From 21384e1fe1975935b3b36395a3fb8a570a1d6e89 Mon Sep 17 00:00:00 2001 From: Sergiu Costea Date: Tue, 30 Apr 2019 17:18:31 +0200 Subject: [PATCH 2/3] Fix integration test; add some missing unit tests --- go/integration/cert_req/main.go | 3 ++ go/lib/infra/messenger/addr_test.go | 75 +++++++++++++++++++++++++---- 2 files changed, 68 insertions(+), 10 deletions(-) diff --git a/go/integration/cert_req/main.go b/go/integration/cert_req/main.go index 3a74478065..29643759ae 100644 --- a/go/integration/cert_req/main.go +++ b/go/integration/cert_req/main.go @@ -77,6 +77,9 @@ func (c client) run() int { messenger.DefaultAdapter, log.Root(), ), + AddressRewriter: &messenger.AddressRewriter{ + Router: &snet.BaseRouter{IA: integration.Local.IA}, + }, }, ) if err = getRemote(); err != nil { diff --git a/go/lib/infra/messenger/addr_test.go b/go/lib/infra/messenger/addr_test.go index a5807b9c16..2b931e5134 100644 --- a/go/lib/infra/messenger/addr_test.go +++ b/go/lib/infra/messenger/addr_test.go @@ -155,11 +155,12 @@ func TestBuildFullAddress(t *testing.T) { func TestResolveIfSVC(t *testing.T) { testCases := []struct { - Description string - InputAddress *addr.AppAddr - ResolverSetup func(*mock_messenger.MockResolver) - ExpectedAddress *addr.AppAddr - ExpectedError bool + Description string + InputAddress *addr.AppAddr + ResolverSetup func(*mock_messenger.MockResolver) + SVCResolutionFraction float64 + ExpectedAddress *addr.AppAddr + ExpectedError bool }{ { Description: "non-svc address does not trigger lookup", @@ -167,11 +168,24 @@ func TestResolveIfSVC(t *testing.T) { L3: addr.HostFromIP(net.IP{192, 168, 0, 1}), L4: addr.NewL4UDPInfo(1), }, + SVCResolutionFraction: 1.0, ExpectedAddress: &addr.AppAddr{ L3: addr.HostFromIP(net.IP{192, 168, 0, 1}), L4: addr.NewL4UDPInfo(1), }, }, + { + Description: "disabling SVC resolution does not trigger lookup, same addr is returned", + InputAddress: &addr.AppAddr{ + L3: addr.SvcBS, + L4: addr.NewL4UDPInfo(1), + }, + SVCResolutionFraction: 0.0, + ExpectedAddress: &addr.AppAddr{ + L3: addr.SvcBS, + L4: addr.NewL4UDPInfo(1), + }, + }, { Description: "svc address, lookup fails", InputAddress: &addr.AppAddr{ @@ -182,7 +196,24 @@ func TestResolveIfSVC(t *testing.T) { r.EXPECT().LookupSVC(gomock.Any(), gomock.Any(), gomock.Any()). Return(nil, fmt.Errorf("err")) }, - ExpectedError: true, + SVCResolutionFraction: 1.0, + ExpectedError: true, + }, + { + Description: "svc address, half time allowed for resolution, lookup fails", + InputAddress: &addr.AppAddr{ + L3: addr.SvcBS, + L4: addr.NewL4UDPInfo(1), + }, + ResolverSetup: func(r *mock_messenger.MockResolver) { + r.EXPECT().LookupSVC(gomock.Any(), gomock.Any(), gomock.Any()). + Return(nil, fmt.Errorf("err")) + }, + SVCResolutionFraction: 0.5, + ExpectedAddress: &addr.AppAddr{ + L3: addr.SvcBS, + L4: addr.NewL4UDPInfo(1), + }, }, { Description: "svc address, lookup succeeds", @@ -201,6 +232,30 @@ func TestResolveIfSVC(t *testing.T) { nil, ) }, + SVCResolutionFraction: 1.0, + ExpectedAddress: &addr.AppAddr{ + L3: addr.HostFromIP(net.IP{192, 168, 1, 1}), + L4: addr.NewL4UDPInfo(8000), + }, + }, + { + Description: "svc address, half time allowed for resolution, lookup succeeds", + InputAddress: &addr.AppAddr{ + L3: addr.SvcBS, + L4: addr.NewL4UDPInfo(1), + }, + ResolverSetup: func(r *mock_messenger.MockResolver) { + r.EXPECT().LookupSVC(gomock.Any(), gomock.Any(), gomock.Any()). + Return( + &svc.Reply{ + Transports: map[svc.Transport]string{ + svc.UDP: "192.168.1.1:8000", + }, + }, + nil, + ) + }, + SVCResolutionFraction: 0.5, ExpectedAddress: &addr.AppAddr{ L3: addr.HostFromIP(net.IP{192, 168, 1, 1}), L4: addr.NewL4UDPInfo(8000), @@ -214,12 +269,12 @@ func TestResolveIfSVC(t *testing.T) { resolver := mock_messenger.NewMockResolver(ctrl) path := mock_snet.NewMockPath(ctrl) path.EXPECT().Destination().Return(addr.IA{}).AnyTimes() - aw := AddressRewriter{ - Resolver: resolver, - SVCResolutionFraction: 1.0, - } for _, tc := range testCases { Convey(tc.Description, func() { + aw := AddressRewriter{ + Resolver: resolver, + SVCResolutionFraction: tc.SVCResolutionFraction, + } initResolver(resolver, tc.ResolverSetup) a, err := aw.resolveIfSVC(context.Background(), path, tc.InputAddress) SoMsg("addr", a, ShouldResemble, tc.ExpectedAddress) From 9537a02e282cbffecc5c8fc927f380ea3dc49a57 Mon Sep 17 00:00:00 2001 From: Sergiu Costea Date: Fri, 3 May 2019 11:12:53 +0200 Subject: [PATCH 3/3] feedback #0 --- go/lib/infra/messenger/addr.go | 26 ++++++++++---------------- go/lib/infra/messenger/messenger.go | 4 ++-- 2 files changed, 12 insertions(+), 18 deletions(-) diff --git a/go/lib/infra/messenger/addr.go b/go/lib/infra/messenger/addr.go index b5622e590c..f17752d4dd 100644 --- a/go/lib/infra/messenger/addr.go +++ b/go/lib/infra/messenger/addr.go @@ -33,8 +33,8 @@ type Resolver interface { LookupSVC(ctx context.Context, path snet.Path, svc addr.HostSVC) (*svc.Reply, error) } -// AddressRewriter is used to compute paths and unicast addresses for SVC -// destinations. +// AddressRewriter is used to compute paths and replace SVC destinations with +// unicast addresses. type AddressRewriter struct { // Router obtains path information to fill in address paths, if they are // required and missing. @@ -56,7 +56,7 @@ type AddressRewriter struct { SVCResolutionFraction float64 } -// Rewrite takes address a and adds a path (if one does not already exist but +// Rewrite takes an address and adds a path (if one does not already exist but // is required), and replaces SVC destinations with unicast ones, if desired. func (r AddressRewriter) Rewrite(ctx context.Context, a net.Addr) (net.Addr, error) { // FIXME(scrye): This is not legitimate use. It's only included for @@ -115,15 +115,16 @@ func (r AddressRewriter) buildFullAddress(ctx context.Context, a net.Addr) (*sne return newAddr, nil } -// resolveIfSvc returns an UDP/IP address if the input address is an SVC -// destination. 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. +// resolveIfSvc performs SVC resolution and returns an UDP/IP address if the +// input address is an SVC destination. 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. func (r AddressRewriter) resolveIfSVC(ctx context.Context, p snet.Path, address *addr.AppAddr) (*addr.AppAddr, error) { - svcAddress := getSVC(address.L3) - if svcAddress == addr.SvcNone { + svcAddress, ok := address.L3.(addr.HostSVC) + if !ok { return address.Copy(), nil } if r.SVCResolutionFraction <= 0.0 { @@ -166,13 +167,6 @@ func (r AddressRewriter) resolutionCtx(ctx context.Context) (context.Context, co return context.WithTimeout(ctx, timeout) } -func getSVC(a addr.HostAddr) addr.HostSVC { - if svcAddress, ok := a.(addr.HostSVC); ok { - return svcAddress - } - return addr.SvcNone -} - // parseReply searches for a UDP server on the remote address. If one is not // found, an error is returned. func parseReply(reply *svc.Reply) (*addr.AppAddr, error) { diff --git a/go/lib/infra/messenger/messenger.go b/go/lib/infra/messenger/messenger.go index e4b37cb02a..952f31accc 100644 --- a/go/lib/infra/messenger/messenger.go +++ b/go/lib/infra/messenger/messenger.go @@ -107,8 +107,8 @@ type Config struct { Dispatcher *disp.Dispatcher // TrustStore stores and retrieves certificate chains and TRCs. TrustStore infra.TrustStore - // AddressRewriter is used to compute paths and unicast addresses for SVC - // destinations. AddressRewriter must not be nil. + // AddressRewriter is used to compute paths and replace SVC destinations with + // unicast addresses. AddressRewriter *AddressRewriter // HandlerTimeout is the amount of time allocated to the processing of a // received message. This includes the time needed to verify the signature