From 0b19cd92b94330dd2b65442abb581774b0d16d0f Mon Sep 17 00:00:00 2001 From: JordiSubira Date: Tue, 16 May 2023 17:29:43 +0200 Subject: [PATCH] comments, leftovers, small fixes more minor --- acceptance/sig_short_exp_time/test | 1 + .../1-ff00_0_110/dispatcher/disp.toml | 2 - .../1-ff00_0_111/dispatcher/disp.toml | 2 - gateway/gateway.go | 20 ----- gateway/pathhealth/pathwatcher.go | 9 +-- pkg/snet/BUILD.bazel | 2 +- pkg/snet/{dispatcher.go => scmp.go} | 0 pkg/snet/snet.go | 13 ---- private/app/appnet/infraenv.go | 7 +- private/app/flag/env.go | 3 +- private/svc/svc.go | 8 +- private/topology/topology.go | 3 +- private/underlay/conn/BUILD.bazel | 13 +++- private/underlay/conn/conn.go | 21 +++-- private/underlay/conn/conn_test.go | 78 +++++++++++++++++++ router/dataplane.go | 10 +-- tools/end2end/main.go | 1 - 17 files changed, 123 insertions(+), 70 deletions(-) delete mode 100644 acceptance/sig_short_exp_time/testdata/1-ff00_0_110/dispatcher/disp.toml delete mode 100644 acceptance/sig_short_exp_time/testdata/1-ff00_0_111/dispatcher/disp.toml rename pkg/snet/{dispatcher.go => scmp.go} (100%) create mode 100644 private/underlay/conn/conn_test.go diff --git a/acceptance/sig_short_exp_time/test b/acceptance/sig_short_exp_time/test index 7d0f5e67ab..e2988720c0 100755 --- a/acceptance/sig_short_exp_time/test +++ b/acceptance/sig_short_exp_time/test @@ -10,6 +10,7 @@ # short lifetime expires, the first path is taken down and a ping sent out. If # the failover was correct, this ping should not be lost. +# TODO(JordiSubira): Remove dispatcher from schema. # Docker topology: # # +---------------------------+ +---------------------------+ diff --git a/acceptance/sig_short_exp_time/testdata/1-ff00_0_110/dispatcher/disp.toml b/acceptance/sig_short_exp_time/testdata/1-ff00_0_110/dispatcher/disp.toml deleted file mode 100644 index 2a93d6d8b2..0000000000 --- a/acceptance/sig_short_exp_time/testdata/1-ff00_0_110/dispatcher/disp.toml +++ /dev/null @@ -1,2 +0,0 @@ -[log.console] -level = "debug" diff --git a/acceptance/sig_short_exp_time/testdata/1-ff00_0_111/dispatcher/disp.toml b/acceptance/sig_short_exp_time/testdata/1-ff00_0_111/dispatcher/disp.toml deleted file mode 100644 index 2a93d6d8b2..0000000000 --- a/acceptance/sig_short_exp_time/testdata/1-ff00_0_111/dispatcher/disp.toml +++ /dev/null @@ -1,2 +0,0 @@ -[log.console] -level = "debug" diff --git a/gateway/gateway.go b/gateway/gateway.go index 84ab960c1c..a2411a0116 100644 --- a/gateway/gateway.go +++ b/gateway/gateway.go @@ -106,26 +106,6 @@ func (pcf PacketConnFactory) New() (net.PacketConn, error) { return conn, nil } -type ProbeConnFactory struct { - LocalIA addr.IA - LocalIP net.IP -} - -func (f ProbeConnFactory) New(ctx context.Context) (net.PacketConn, error) { - pathMonitorConnection, err := (&snet.SCIONNetwork{ - LocalIA: f.LocalIA, - Connector: &snet.DefaultConnector{ - SCMPHandler: ignoreSCMP{}, - }, - }).Listen(ctx, "udp", &net.UDPAddr{IP: f.LocalIP}, addr.SvcNone) - if err != nil { - return nil, serrors.WrapStr("unable to open control socket", err) - } - log.FromCtx(ctx).Debug("Path monitor connection opened on Raw UDP/SCION", - "local_addr", pathMonitorConnection.LocalAddr()) - return pathMonitorConnection, nil -} - type RoutingTableFactory struct { RoutePublisherFactory control.PublisherFactory } diff --git a/gateway/pathhealth/pathwatcher.go b/gateway/pathhealth/pathwatcher.go index fa27d64558..6a15131193 100644 --- a/gateway/pathhealth/pathwatcher.go +++ b/gateway/pathhealth/pathwatcher.go @@ -37,12 +37,6 @@ const ( defaultProbeInterval = 500 * time.Millisecond ) -// ProbeConnFactory is used to construct net.PacketConn objects for sending and -// receiving probes. -type ProbeConnFactory interface { - New(context.Context) (net.PacketConn, error) -} - // DefaultPathWatcherFactory creates PathWatchers. type DefaultPathWatcherFactory struct { // LocalIA is the ID of the local AS. @@ -51,7 +45,6 @@ type DefaultPathWatcherFactory struct { LocalIP net.IP // RevocationHandler is the revocation handler. RevocationHandler snet.RevocationHandler - // ConnFactory is used to create probe connections. // Probeinterval defines the interval at which probes are sent. If it is not // set a default is used. ProbeInterval time.Duration @@ -86,6 +79,8 @@ func (f *DefaultPathWatcherFactory) New( } return create(remote) } + // FIXME(JordiSubira): Keep or change the listening port, once we decide + // how SCMP are addressed. nc, err := (&snet.DefaultConnector{ SCMPHandler: scmpHandler{ wrappedHandler: snet.DefaultSCMPHandler{ diff --git a/pkg/snet/BUILD.bazel b/pkg/snet/BUILD.bazel index ab40cc2274..5194fd472b 100644 --- a/pkg/snet/BUILD.bazel +++ b/pkg/snet/BUILD.bazel @@ -5,7 +5,6 @@ go_library( srcs = [ "base.go", "conn.go", - "dispatcher.go", "interface.go", "packet.go", "packet_conn.go", @@ -13,6 +12,7 @@ go_library( "reader.go", "reply_pather.go", "router.go", + "scmp.go", "snet.go", "svcaddr.go", "udpaddr.go", diff --git a/pkg/snet/dispatcher.go b/pkg/snet/scmp.go similarity index 100% rename from pkg/snet/dispatcher.go rename to pkg/snet/scmp.go diff --git a/pkg/snet/snet.go b/pkg/snet/snet.go index a15324f22c..5842e53c3a 100644 --- a/pkg/snet/snet.go +++ b/pkg/snet/snet.go @@ -127,19 +127,6 @@ func (n *SCIONNetwork) Listen(ctx context.Context, network string, listen *net.U return nil, serrors.New("Unknown network", "network", network) } - // FIXME(JordiSubira): Without the dispatcher if the host in the address parameter is nil - // or a literal unspecified IP address we can listen on all available IP addresses, except - // multicast. Port and address can be exposed using LocalAddr() on the returned Conn. - // if listen == nil { - // return nil, serrors.New("nil listen addr not supported") - // } - // if listen.IP == nil { - // return nil, serrors.New("nil listen IP not supported") - // } - // if listen.IP.IsUnspecified() { - // return nil, serrors.New("unspecified listen IP not supported") - // } - packetConn, err := n.Connector.OpenUDP(listen) if err != nil { return nil, err diff --git a/private/app/appnet/infraenv.go b/private/app/appnet/infraenv.go index 622f7a9870..1ffce404af 100644 --- a/private/app/appnet/infraenv.go +++ b/private/app/appnet/infraenv.go @@ -203,8 +203,8 @@ func GenerateTLSConfig() (*tls.Config, error) { // AddressRewriter initializes path and svc resolvers for infra servers. // -// The connection factory is used to open sockets for SVC resolution requests. -// If the connection factory is nil, the default connection factory is used. +// The connector is used to open sockets for SVC resolution requests. +// If the connector is nil, the default connection factory is used. func (nc *NetworkConfig) AddressRewriter( connector snet.Connector) *AddressRewriter { @@ -291,6 +291,9 @@ func (nc *NetworkConfig) initQUICSockets() (net.PacketConn, net.PacketConn, erro serverNet := &snet.SCIONNetwork{ LocalIA: nc.IA, Connector: &snet.DefaultConnector{ + // XXX(roosd): This is essential, the server must not read SCMP + // errors. Otherwise, the accept loop will always return that error + // on every subsequent call to accept. SCMPHandler: ignoreSCMP{}, Metrics: nc.SCIONPacketConnMetrics, }, diff --git a/private/app/flag/env.go b/private/app/flag/env.go index a53b2452db..9689ec4db2 100644 --- a/private/app/flag/env.go +++ b/private/app/flag/env.go @@ -75,8 +75,7 @@ func (v *ipVal) Type() string { return "ip" } func (v *ipVal) String() string { return netip.Addr(*v).String() } // SCIONEnvironment can be used to access the common SCION configuration values, -// like the SCION daemon address and the local IP -// as well as the local ISD-AS. +// like the SCION daemon address and the local IP as well as the local ISD-AS. type SCIONEnvironment struct { sciondFlag *pflag.Flag sciondEnv *string diff --git a/private/svc/svc.go b/private/svc/svc.go index 1fb8cc60af..d3cd23dd8a 100644 --- a/private/svc/svc.go +++ b/private/svc/svc.go @@ -50,9 +50,9 @@ const ( type ResolverPacketConnector struct { // Connector opens a PacketConn to receive and send packets. Connector snet.Connector - // source contains the address from which packets should be sent. + // LocalIA contains the address from which packets should be sent. LocalIA addr.IA - // handler handles packets for SVC destinations. + // Handler handles packets for SVC destinations. Handler RequestHandler } @@ -76,9 +76,9 @@ func (c *ResolverPacketConnector) OpenUDP(u *net.UDPAddr) (snet.PacketConn, erro type ResolverPacketConn struct { // PacketConn is the conn to receive and send packets. snet.PacketConn - // source contains the address from which packets should be sent. + // Source contains the address from which packets should be sent. Source snet.SCIONAddress - // handler handles packets for SVC destinations. + // Handler handles packets for SVC destinations. Handler RequestHandler } diff --git a/private/topology/topology.go b/private/topology/topology.go index 6fa8bd55b2..99fb9c91f3 100644 --- a/private/topology/topology.go +++ b/private/topology/topology.go @@ -33,7 +33,8 @@ import ( const ( // EndhostPort is the underlay port that SCION binds to on non-routers. - EndhostPort = underlay.EndhostPort + EndhostPort = underlay.EndhostPort + // TODO(JordiSubira): Yet to be defined. HostPortRangeLow = 0 HostPortRangeHigh = 1<<16 - 1 ) diff --git a/private/underlay/conn/BUILD.bazel b/private/underlay/conn/BUILD.bazel index 3c0395a865..7236035079 100644 --- a/private/underlay/conn/BUILD.bazel +++ b/private/underlay/conn/BUILD.bazel @@ -1,4 +1,4 @@ -load("//tools/lint:go.bzl", "go_library") +load("//tools/lint:go.bzl", "go_library", "go_test") go_library( name = "go_default_library", @@ -23,3 +23,14 @@ go_library( "//conditions:default": [], }), ) + +go_test( + name = "go_default_test", + srcs = ["conn_test.go"], + embed = [":go_default_library"], + deps = [ + "//pkg/private/xtest:go_default_library", + "@com_github_stretchr_testify//assert:go_default_library", + "@com_github_stretchr_testify//require:go_default_library", + ], +) diff --git a/private/underlay/conn/conn.go b/private/underlay/conn/conn.go index 4c87304532..f5d96275aa 100644 --- a/private/underlay/conn/conn.go +++ b/private/underlay/conn/conn.go @@ -32,6 +32,13 @@ import ( "github.com/scionproto/scion/private/underlay/sockctrl" ) +const ( + // ReceiveBufferSize is the size of receive buffers used by the dispatcher. + ReceiveBufferSize = 1 << 20 + // SendBufferSize is the size of the send buffers used by the dispatcher. + SendBufferSize = 1 << 20 +) + // Messages is a list of ipX.Messages. It is necessary to hide the type alias // between ipv4.Message, ipv6.Message and socket.Message. type Messages []ipv4.Message @@ -88,6 +95,11 @@ func OpenConn(addr *net.UDPAddr) (net.PacketConn, error) { // because the socket options are specific to only one socket type, so we // degrade udp to only udp4. listeningAddr := copyUDPAddr(addr) + if listeningAddr == nil { + listeningAddr = &net.UDPAddr{ + IP: net.IPv4zero, + } + } if (listeningAddr.Network() == "udp" || listeningAddr.Network() == "udp4") && listeningAddr.IP == nil { listeningAddr.IP = net.IPv4zero @@ -96,9 +108,11 @@ func OpenConn(addr *net.UDPAddr) (net.PacketConn, error) { listeningAddr.IP = net.IPv6zero } + // TODO(JordiSubira): Should we keep a default config or use the passed-through + // configuration. c, err := New(listeningAddr, nil, &Config{ - SendBufferSize: 0, - ReceiveBufferSize: 0, + SendBufferSize: SendBufferSize, + ReceiveBufferSize: ReceiveBufferSize, }) if err != nil { return nil, serrors.WrapStr("unable to open conn", err) @@ -193,9 +207,6 @@ type connUDPBase struct { func (cc *connUDPBase) initConnUDP(network string, laddr, raddr *net.UDPAddr, cfg *Config) error { var c *net.UDPConn var err error - if laddr == nil { - return serrors.New("listen address must be specified") - } if raddr == nil { if c, err = net.ListenUDP(network, laddr); err != nil { return serrors.WrapStr("Error listening on socket", err, diff --git a/private/underlay/conn/conn_test.go b/private/underlay/conn/conn_test.go new file mode 100644 index 0000000000..f54ec78ed1 --- /dev/null +++ b/private/underlay/conn/conn_test.go @@ -0,0 +1,78 @@ +// Copyright 2023 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 conn + +import ( + "net" + "testing" + + "github.com/scionproto/scion/pkg/private/xtest" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestOpenConn(t *testing.T) { + testCases := map[string]struct { + addr *net.UDPAddr + }{ + "nil": { + addr: nil, + }, + "undefined_addr": { + addr: xtest.MustParseUDPAddr(t, "0.0.0.0:0"), + }, + "undefined_port": { + addr: xtest.MustParseUDPAddr(t, "127.0.0.1:0"), + }, + } + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + sc, err := OpenConn(tc.addr) + require.NoError(t, err) + defer sc.Close() + lAddr := sc.LocalAddr().(*net.UDPAddr) + + if tc.addr != nil && !tc.addr.IP.IsUnspecified() { + assert.Equal(t, tc.addr.IP, lAddr.IP) + } + + // Client + cc, err := New(nil, lAddr, &Config{ + SendBufferSize: 0, + ReceiveBufferSize: 0, + }) + require.NoError(t, err) + defer cc.Close() + + exchangeMessages := func(sc net.PacketConn, cc Conn) { + msg := []byte("hello") + + go func() { + _, err := cc.Write(msg) + require.NoError(t, err) + }() + + buf := make([]byte, 100) + n, cAddr, err := sc.ReadFrom(buf) + require.NoError(t, err) + assert.Equal(t, msg, buf[:n]) + assert.Equal(t, cc.LocalAddr(), cAddr) + } + + exchangeMessages(sc, cc) + }) + } + +} diff --git a/router/dataplane.go b/router/dataplane.go index 5e135baf3d..c8dc8b034c 100644 --- a/router/dataplane.go +++ b/router/dataplane.go @@ -624,14 +624,6 @@ func (p *scionPacketProcessor) processPkt(rawPkt []byte, return processResult{}, err } pld := p.lastLayer.LayerPayload() - srcHost, _ := p.scionLayer.SrcAddr() - dstHost, _ := p.scionLayer.DstAddr() - log.Debug("packet", - "src IA", p.scionLayer.SrcIA, - "dst IA", p.scionLayer.DstIA, - "src addr", srcHost.String(), - "dst addr", dstHost.String(), - ) pathType := p.scionLayer.PathType switch pathType { @@ -1466,9 +1458,9 @@ func addEndhostPort(lastLayer gopacket.DecodingLayer, dst *net.IPAddr) (*net.UDP if port < topology.HostPortRangeLow || port > topology.HostPortRangeHigh { port = topology.EndhostPort } - log.Debug("TBR XXXJ:", "udp port that will be rewritten", port) return &net.UDPAddr{IP: dst.IP, Port: int(port)}, nil case slayers.L4SCMP: + // TODO(JordiSubira): On-going discussion regarding SCMP dst port return &net.UDPAddr{IP: dst.IP, Port: topology.EndhostPort}, nil default: panic(fmt.Sprintf("Port rewriting not supported for protcol number %v", l4Type)) diff --git a/tools/end2end/main.go b/tools/end2end/main.go index 165288fde2..5930c905c4 100644 --- a/tools/end2end/main.go +++ b/tools/end2end/main.go @@ -203,7 +203,6 @@ func (s server) handlePing(conn snet.PacketConn) error { )) } log.Info(fmt.Sprintf("Ping received from %s:%d, sending pong.", p.Source, udp.SrcPort)) - log.Info((fmt.Sprintf("%v %d", p.Source, udp.SrcPort))) raw, err := json.Marshal(Pong{ Client: p.Source.IA, Server: integration.Local.IA,