From 9c513314d05d973c3098b1536c2c8bcc31f12e8a Mon Sep 17 00:00:00 2001 From: Matthias Frei Date: Tue, 2 Jul 2024 10:49:02 +0200 Subject: [PATCH] private/appnet: remove SvcResolutionFraction Clean up unused code paths related to old pre-QUIC-RPC-era service address resolution and the ominous SvcResolutionFraction. --- control/onehop/addr.go | 4 +- gateway/gateway.go | 1 - pkg/grpc/dialer.go | 4 +- private/app/appnet/addr.go | 90 +++-------------- private/app/appnet/addr_test.go | 157 ++++++------------------------ private/app/appnet/export_test.go | 2 +- private/app/appnet/infraenv.go | 1 - scion-pki/certs/renew.go | 1 - 8 files changed, 48 insertions(+), 212 deletions(-) diff --git a/control/onehop/addr.go b/control/onehop/addr.go index 6202443da5..0481d1e600 100644 --- a/control/onehop/addr.go +++ b/control/onehop/addr.go @@ -61,14 +61,14 @@ type AddressRewriter struct { func (r *AddressRewriter) RedirectToQUIC( ctx context.Context, address net.Addr, -) (net.Addr, bool, error) { +) (net.Addr, error) { a, ok := address.(*Addr) if !ok { return r.Rewriter.RedirectToQUIC(ctx, address) } path, err := r.getPath(a.Egress) if err != nil { - return nil, false, err + return nil, err } svc := &snet.SVCAddr{ IA: a.IA, diff --git a/gateway/gateway.go b/gateway/gateway.go index 31d1448dfe..5ad5b84530 100644 --- a/gateway/gateway.go +++ b/gateway/gateway.go @@ -505,7 +505,6 @@ func (g *Gateway) Run(ctx context.Context) error { Network: scionNetwork, LocalIP: g.ServiceDiscoveryClientIP, }, - SVCResolutionFraction: 1.337, }, }, }, diff --git a/pkg/grpc/dialer.go b/pkg/grpc/dialer.go index 03edee33ad..abaf419852 100644 --- a/pkg/grpc/dialer.go +++ b/pkg/grpc/dialer.go @@ -122,7 +122,7 @@ func (t *TCPDialer) Dial(ctx context.Context, dst net.Addr) (*grpc.ClientConn, e // AddressRewriter redirects to QUIC endpoints. type AddressRewriter interface { - RedirectToQUIC(ctx context.Context, address net.Addr) (net.Addr, bool, error) + RedirectToQUIC(ctx context.Context, address net.Addr) (net.Addr, error) } // ConnDialer dials a net.Conn. @@ -143,7 +143,7 @@ func (d *QUICDialer) Dial(ctx context.Context, addr net.Addr) (*grpc.ClientConn, // resolver+balancer mechanism of gRPC. For now, keep the legacy behavior of // dialing a connection based on the QUIC redirects. - addr, _, err := d.Rewriter.RedirectToQUIC(ctx, addr) + addr, err := d.Rewriter.RedirectToQUIC(ctx, addr) if err != nil { return nil, serrors.WrapStr("resolving SVC address", err) } diff --git a/private/app/appnet/addr.go b/private/app/appnet/addr.go index 2d5514c40b..f659fbd410 100644 --- a/private/app/appnet/addr.go +++ b/private/app/appnet/addr.go @@ -16,10 +16,8 @@ package appnet import ( "context" - "errors" "fmt" "net" - "time" "github.com/scionproto/scion/pkg/addr" "github.com/scionproto/scion/pkg/log" @@ -47,81 +45,41 @@ type AddressRewriter struct { SVCRouter SVCResolver // 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 } // RedirectToQUIC takes an address and adds a path (if one does not already // exist but is required), and replaces SVC destinations with QUIC unicast // ones, if possible. // -// The returned boolean value is set to true if the remote server is -// QUIC-compatible and we have successfully discovered its address. -// // If the address is already unicast, no redirection to QUIC is attempted. func (r AddressRewriter) RedirectToQUIC(ctx context.Context, - address net.Addr) (net.Addr, bool, error) { - logger := log.FromCtx(ctx) - - // 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 address == nil { - return address, false, nil - } + address net.Addr) (net.Addr, error) { switch a := address.(type) { case *snet.UDPAddr: - return a, false, nil + return a, nil case *snet.SVCAddr: fa, err := r.buildFullAddress(ctx, a) if err != nil { - return nil, false, err - } - if r.SVCResolutionFraction <= 0.0 { - return fa, false, nil + return nil, err } path, err := fa.GetPath() if err != nil { - return nil, false, serrors.WrapStr("bad path", err) + return nil, serrors.WrapStr("bad path", err) } // During One-Hop Path operation, use SVC resolution to also bootstrap the path. - p, u, quicRedirect, err := r.resolveSVC(ctx, path, fa.SVC) + p, u, err := r.resolveSVC(ctx, path, fa.SVC) if err != nil { - // For a revoked path we don't fallback we want to give the option - // to retry with a new path. - isRevokedPath := func(err error) bool { - var opErr *snet.OpError - return errors.As(err, &opErr) && opErr.RevInfo() != nil - } - if r.SVCResolutionFraction < 1.0 && !isRevokedPath(err) { - // 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.Debug("Falling back to legacy mode, ignore error", "err", err) - return fa, false, nil - } - return a, false, err + return a, err } ret := &snet.UDPAddr{IA: fa.IA, Path: p.Dataplane(), NextHop: fa.NextHop, Host: u} - return ret, quicRedirect, err + return ret, nil } - return nil, false, serrors.New("address type not supported", + return nil, serrors.New("address type not supported", "addr", fmt.Sprintf("%v(%T)", address, address)) } @@ -167,48 +125,30 @@ func (r AddressRewriter) buildFullAddress(ctx context.Context, return ret, 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 +// resolveSVC performs SVC resolution and returns an UDP/IP address. +// 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.SVC) (snet.Path, *net.UDPAddr, bool, error) { + s addr.SVC) (snet.Path, *net.UDPAddr, error) { logger := log.FromCtx(ctx) - if r.SVCResolutionFraction < 1.0 { - var cancelF context.CancelFunc - ctx, cancelF = r.resolutionCtx(ctx) - defer cancelF() - } - logger.Debug("Sending SVC resolution request", "isd_as", p.Destination(), "svc", s, - "svcResFraction", r.SVCResolutionFraction) + logger.Debug("Sending SVC resolution request", "isd_as", p.Destination(), "svc", s) reply, err := r.Resolver.LookupSVC(ctx, p, s) if err != nil { logger.Debug("SVC resolution failed", "err", err) - return nil, nil, false, err + return nil, nil, err } logger.Debug("SVC resolution successful", "reply", reply) u, err := parseReply(reply) if err != nil { - return nil, nil, false, err - } - return reply.ReturnPath, u, true, nil -} - -func (r AddressRewriter) resolutionCtx(ctx context.Context) (context.Context, context.CancelFunc) { - deadline, ok := ctx.Deadline() - if !ok { - return context.WithCancel(ctx) + return nil, nil, err } - - timeout := time.Until(deadline) - timeout = time.Duration(float64(timeout) * r.SVCResolutionFraction) - return context.WithTimeout(ctx, timeout) + return reply.ReturnPath, u, nil } // parseReply searches for a QUIC server on the remote address. If one is not diff --git a/private/app/appnet/addr_test.go b/private/app/appnet/addr_test.go index dd6a009d3d..b3f482830b 100644 --- a/private/app/appnet/addr_test.go +++ b/private/app/appnet/addr_test.go @@ -37,30 +37,24 @@ import ( func TestRedirectQUIC(t *testing.T) { dummyIA := xtest.MustParseIA("1-ff00:0:2") testCases := map[string]struct { - input net.Addr - wantAddr net.Addr - wantRedirect bool - SVCResolutionFraction float64 - assertErr assert.ErrorAssertionFunc + input net.Addr + wantAddr net.Addr + assertErr assert.ErrorAssertionFunc }{ - "nil addr, no error": { + "nil addr, error": { input: nil, wantAddr: nil, - assertErr: assert.NoError, + assertErr: assert.Error, }, "not nil invalid addr, error": { - input: &net.TCPAddr{}, - wantAddr: nil, - wantRedirect: false, - SVCResolutionFraction: 1.0, - assertErr: assert.Error, + input: &net.TCPAddr{}, + wantAddr: nil, + assertErr: assert.Error, }, "valid UDPAddr, returns unchanged": { - input: &snet.UDPAddr{IA: dummyIA, Host: &net.UDPAddr{}}, - wantAddr: &snet.UDPAddr{IA: dummyIA, Host: &net.UDPAddr{}}, - wantRedirect: false, - SVCResolutionFraction: 1.0, - assertErr: assert.NoError, + input: &snet.UDPAddr{IA: dummyIA, Host: &net.UDPAddr{}}, + wantAddr: &snet.UDPAddr{IA: dummyIA, Host: &net.UDPAddr{}}, + assertErr: assert.NoError, }, } for tn, tc := range testCases { @@ -73,52 +67,16 @@ func TestRedirectQUIC(t *testing.T) { resolver.EXPECT().LookupSVC(gomock.Any(), gomock.Any(), gomock.Any()).Times(0) aw := infraenv.AddressRewriter{ - Resolver: resolver, - Router: router, - SVCResolutionFraction: tc.SVCResolutionFraction, + Resolver: resolver, + Router: router, } - a, r, err := aw.RedirectToQUIC(context.Background(), tc.input) + a, err := aw.RedirectToQUIC(context.Background(), tc.input) tc.assertErr(t, err) assert.Equal(t, tc.wantAddr, a) - assert.Equal(t, tc.wantRedirect, r) }) } - t.Run("Fraction 0.5, returns no error", func(t *testing.T) { - t.Log("svc address, half time allowed for resolution, lookup fails") - ctrl := gomock.NewController(t) - defer ctrl.Finish() - router := mock_snet.NewMockRouter(ctrl) - resolver := mock_infraenv.NewMockResolver(ctrl) - resolver.EXPECT().LookupSVC(gomock.Any(), gomock.Any(), gomock.Any()). - Return(nil, fmt.Errorf("lookups errors")) - path := mock_snet.NewMockPath(ctrl) - router.EXPECT().Route(gomock.Any(), gomock.Any()).Return(path, nil) - path.EXPECT().Dataplane().Return(snetpath.SCION{}) - path.EXPECT().UnderlayNextHop().Return(&net.UDPAddr{IP: net.ParseIP("10.1.1.1")}) - path.EXPECT().Metadata().Return(&snet.PathMetadata{ - Interfaces: make([]snet.PathInterface, 1), // just non-empty - }) - aw := infraenv.AddressRewriter{ - Router: router, - Resolver: resolver, - SVCResolutionFraction: 0.5, - } - - input := &snet.SVCAddr{IA: dummyIA, SVC: addr.SvcCS, Path: snetpath.Empty{}} - want := &snet.SVCAddr{ - IA: dummyIA, - NextHop: &net.UDPAddr{IP: net.ParseIP("10.1.1.1")}, - SVC: addr.SvcCS, - Path: snetpath.SCION{}, - } - a, r, err := aw.RedirectToQUIC(context.Background(), input) - assert.NoError(t, err) - assert.Equal(t, want, a) - assert.False(t, r) - }) - t.Run("valid SVCAddr, returns no error and UPDAddr", func(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() @@ -137,9 +95,8 @@ func TestRedirectQUIC(t *testing.T) { Interfaces: make([]snet.PathInterface, 1), // just non-empty }) aw := infraenv.AddressRewriter{ - Router: router, - Resolver: resolver, - SVCResolutionFraction: 1, + Router: router, + Resolver: resolver, } input := &snet.SVCAddr{IA: dummyIA, SVC: addr.SvcCS, Path: snetpath.Empty{}} @@ -149,44 +106,11 @@ func TestRedirectQUIC(t *testing.T) { NextHop: &net.UDPAddr{IP: net.ParseIP("10.1.1.1")}, Host: &net.UDPAddr{IP: net.ParseIP("192.168.1.1"), Port: 8000}, } - a, r, err := aw.RedirectToQUIC(context.Background(), input) + a, err := aw.RedirectToQUIC(context.Background(), input) assert.NoError(t, err) assert.Equal(t, want, a) - assert.True(t, r) }) - t.Run("valid SVCAddr, no resolution, resolves path", func(t *testing.T) { - ctrl := gomock.NewController(t) - defer ctrl.Finish() - router := mock_snet.NewMockRouter(ctrl) - - path := mock_snet.NewMockPath(ctrl) - router.EXPECT().Route(gomock.Any(), gomock.Any()).Return(path, nil) - path.EXPECT().Dataplane().Return(snetpath.SCION{}) - path.EXPECT().UnderlayNextHop().Return(&net.UDPAddr{IP: net.ParseIP("10.1.1.1")}) - path.EXPECT().Metadata().Return(&snet.PathMetadata{}) - svcRouter := mock_infraenv.NewMockSVCResolver(ctrl) - svcRouter.EXPECT().GetUnderlay(addr.SvcCS).Return( - &net.UDPAddr{IP: net.ParseIP("10.1.1.1")}, nil, - ) - - aw := infraenv.AddressRewriter{ - Router: router, - SVCRouter: svcRouter, - SVCResolutionFraction: 0.0, - } - - input := &snet.SVCAddr{SVC: addr.SvcCS, Path: snetpath.Empty{}} - want := &snet.SVCAddr{ - SVC: addr.SvcCS, - NextHop: &net.UDPAddr{IP: net.ParseIP("10.1.1.1")}, - Path: snetpath.Empty{}, - } - a, r, err := aw.RedirectToQUIC(context.Background(), input) - assert.NoError(t, err) - assert.Equal(t, want, a) - assert.False(t, r) - }) } func TestBuildFullAddress(t *testing.T) { @@ -321,13 +245,11 @@ func TestBuildFullAddress(t *testing.T) { func TestResolve(t *testing.T) { testCases := map[string]struct { - input addr.SVC - ResolverSetup func(*mock_infraenv.MockResolver) - SVCResolutionFraction float64 - wantPath snet.Path - want *net.UDPAddr - wantQUICRedirect bool - assertErr assert.ErrorAssertionFunc + input addr.SVC + ResolverSetup func(*mock_infraenv.MockResolver) + wantPath snet.Path + want *net.UDPAddr + assertErr assert.ErrorAssertionFunc }{ "svc address, lookup fails": { input: addr.SvcCS, @@ -335,8 +257,7 @@ func TestResolve(t *testing.T) { r.EXPECT().LookupSVC(gomock.Any(), gomock.Any(), gomock.Any()). Return(nil, fmt.Errorf("err")) }, - SVCResolutionFraction: 1.0, - assertErr: assert.Error, + assertErr: assert.Error, }, "svc address, lookup succeeds": { input: addr.SvcCS, @@ -352,29 +273,9 @@ func TestResolve(t *testing.T) { nil, ) }, - SVCResolutionFraction: 1.0, - wantPath: &testPath{}, - 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: addr.SvcCS, - ResolverSetup: func(r *mock_infraenv.MockResolver) { - r.EXPECT().LookupSVC(gomock.Any(), gomock.Any(), gomock.Any()). - Return( - &svc.Reply{ - Transports: map[svc.Transport]string{ - svc.QUIC: "192.168.1.1:8000", - }, - }, - nil, - ) - }, - SVCResolutionFraction: 0.5, - want: &net.UDPAddr{IP: net.ParseIP("192.168.1.1"), Port: 8000}, - wantQUICRedirect: true, - assertErr: assert.NoError, + wantPath: &testPath{}, + want: &net.UDPAddr{IP: net.IP{192, 168, 1, 1}, Port: 8000}, + assertErr: assert.NoError, }, } @@ -386,14 +287,12 @@ func TestResolve(t *testing.T) { path := mock_snet.NewMockPath(ctrl) path.EXPECT().Destination().Return(addr.IA(0)).AnyTimes() aw := infraenv.AddressRewriter{ - Resolver: resolver, - SVCResolutionFraction: tc.SVCResolutionFraction, + Resolver: resolver, } initResolver(resolver, tc.ResolverSetup) - p, a, redirect, err := aw.ResolveSVC(context.Background(), path, tc.input) + p, a, err := aw.ResolveSVC(context.Background(), path, tc.input) assert.Equal(t, tc.wantPath, p) assert.Equal(t, tc.want.String(), a.String()) - assert.Equal(t, tc.wantQUICRedirect, redirect) tc.assertErr(t, err) }) } diff --git a/private/app/appnet/export_test.go b/private/app/appnet/export_test.go index 47e689ebff..09e24ea1c3 100644 --- a/private/app/appnet/export_test.go +++ b/private/app/appnet/export_test.go @@ -29,7 +29,7 @@ func (r AddressRewriter) BuildFullAddress(ctx context.Context, } func (r AddressRewriter) ResolveSVC(ctx context.Context, p snet.Path, - s addr.SVC) (snet.Path, *net.UDPAddr, bool, error) { + s addr.SVC) (snet.Path, *net.UDPAddr, error) { return r.resolveSVC(ctx, p, s) } diff --git a/private/app/appnet/infraenv.go b/private/app/appnet/infraenv.go index b18a7db2dc..7d86d39937 100644 --- a/private/app/appnet/infraenv.go +++ b/private/app/appnet/infraenv.go @@ -205,7 +205,6 @@ func (nc *NetworkConfig) AddressRewriter() *AddressRewriter { }, LocalIP: nc.Public.IP, }, - SVCResolutionFraction: 1.337, } } diff --git a/scion-pki/certs/renew.go b/scion-pki/certs/renew.go index bb7f5449e8..cd13c6bfef 100644 --- a/scion-pki/certs/renew.go +++ b/scion-pki/certs/renew.go @@ -763,7 +763,6 @@ func (r *renewer) requestRemote( Network: sn, LocalIP: local.Host.IP, }, - SVCResolutionFraction: 1, }, Dialer: squic.ConnDialer{ Transport: &quic.Transport{Conn: conn},