Skip to content

Commit

Permalink
don't pass the QUIC version to the StartedConnection event
Browse files Browse the repository at this point in the history 
The version might change in response to a Version Negotiation packet.
  • Loading branch information
@marten-seemann
marten-seemann committed Apr 2, 2021
1 parent ea14ce5 commit c30a45e
Show file tree
Hide file tree
Showing 14 changed files with 34 additions and 47 deletions.
6 changes: 3 additions & 3 deletions client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,9 @@ func dialContext(
if c.config.Tracer != nil {
c.tracer = c.config.Tracer.TracerForConnection(protocol.PerspectiveClient, c.destConnID)
}
if c.tracer != nil {
c.tracer.StartedConnection(c.conn.LocalAddr(), c.conn.RemoteAddr(), c.srcConnID, c.destConnID)
}
if err := c.dial(ctx); err != nil {
return nil, err
}
Expand Down Expand Up @@ -270,9 +273,6 @@ func newClient(

func (c *client) dial(ctx context.Context) error {
c.logger.Infof("Starting new connection to %s (%s -> %s), source connection ID %s, destination connection ID %s, version %s", c.tlsConf.ServerName, c.conn.LocalAddr(), c.conn.RemoteAddr(), c.srcConnID, c.destConnID, c.version)
if c.tracer != nil {
c.tracer.StartedConnection(c.conn.LocalAddr(), c.conn.RemoteAddr(), c.version, c.srcConnID, c.destConnID)
}

c.session = newClientSession(
c.conn,
Expand Down
17 changes: 6 additions & 11 deletions client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ var _ = Describe("Client", func() {
sess.EXPECT().run()
return sess
}
tracer.EXPECT().StartedConnection(packetConn.LocalAddr(), addr, protocol.VersionTLS, gomock.Any(), gomock.Any())
tracer.EXPECT().StartedConnection(packetConn.LocalAddr(), addr, gomock.Any(), gomock.Any())
_, err := Dial(
packetConn,
addr,
Expand Down Expand Up @@ -242,7 +242,7 @@ var _ = Describe("Client", func() {
sess.EXPECT().HandshakeComplete().Return(ctx)
return sess
}
tracer.EXPECT().StartedConnection(gomock.Any(), gomock.Any(), protocol.VersionTLS, gomock.Any(), gomock.Any())
tracer.EXPECT().StartedConnection(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())
s, err := Dial(
packetConn,
addr,
Expand Down Expand Up @@ -287,7 +287,7 @@ var _ = Describe("Client", func() {
go func() {
defer GinkgoRecover()
defer close(done)
tracer.EXPECT().StartedConnection(gomock.Any(), gomock.Any(), protocol.VersionTLS, gomock.Any(), gomock.Any())
tracer.EXPECT().StartedConnection(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())
s, err := DialEarly(
packetConn,
addr,
Expand Down Expand Up @@ -328,7 +328,7 @@ var _ = Describe("Client", func() {
sess.EXPECT().HandshakeComplete().Return(context.Background())
return sess
}
tracer.EXPECT().StartedConnection(gomock.Any(), gomock.Any(), protocol.VersionTLS, gomock.Any(), gomock.Any())
tracer.EXPECT().StartedConnection(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())
_, err := Dial(
packetConn,
addr,
Expand Down Expand Up @@ -371,7 +371,7 @@ var _ = Describe("Client", func() {
dialed := make(chan struct{})
go func() {
defer GinkgoRecover()
tracer.EXPECT().StartedConnection(gomock.Any(), gomock.Any(), protocol.VersionTLS, gomock.Any(), gomock.Any())
tracer.EXPECT().StartedConnection(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())
_, err := DialContext(
ctx,
packetConn,
Expand Down Expand Up @@ -559,8 +559,6 @@ var _ = Describe("Client", func() {
manager.EXPECT().Destroy()
mockMultiplexer.EXPECT().AddConn(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(manager, nil)

initialVersion := cl.version

var counter int
newClientSession = func(
_ sendConn,
Expand Down Expand Up @@ -594,10 +592,7 @@ var _ = Describe("Client", func() {
return sess
}

gomock.InOrder(
tracer.EXPECT().StartedConnection(gomock.Any(), gomock.Any(), initialVersion, gomock.Any(), gomock.Any()),
tracer.EXPECT().StartedConnection(gomock.Any(), gomock.Any(), protocol.VersionNumber(789), gomock.Any(), gomock.Any()),
)
tracer.EXPECT().StartedConnection(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())
_, err := DialAddr("localhost:7890", tlsConf, config)
Expect(err).ToNot(HaveOccurred())
Expect(counter).To(Equal(2))
Expand Down
2 changes: 1 addition & 1 deletion integrationtests/self/self_suite_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,7 @@ type connTracer struct{}

var _ logging.ConnectionTracer = &connTracer{}

func (t *connTracer) StartedConnection(local, remote net.Addr, version logging.VersionNumber, srcConnID, destConnID logging.ConnectionID) {
func (t *connTracer) StartedConnection(local, remote net.Addr, srcConnID, destConnID logging.ConnectionID) {
}
func (t *connTracer) ClosedConnection(logging.CloseReason) {}
func (t *connTracer) SentTransportParameters(*logging.TransportParameters) {}
Expand Down
2 changes: 1 addition & 1 deletion integrationtests/self/tracer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ type customConnTracer struct{}

var _ logging.ConnectionTracer = &customConnTracer{}

func (t *customConnTracer) StartedConnection(local, remote net.Addr, version logging.VersionNumber, srcConnID, destConnID logging.ConnectionID) {
func (t *customConnTracer) StartedConnection(local, remote net.Addr, srcConnID, destConnID logging.ConnectionID) {
}
func (t *customConnTracer) ClosedConnection(logging.CloseReason) {}
func (t *customConnTracer) SentTransportParameters(*logging.TransportParameters) {}
Expand Down
8 changes: 4 additions & 4 deletions internal/mocks/logging/connection_tracer.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion logging/interface.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ type Tracer interface {

// A ConnectionTracer records events.
type ConnectionTracer interface {
StartedConnection(local, remote net.Addr, version VersionNumber, srcConnID, destConnID ConnectionID)
StartedConnection(local, remote net.Addr, srcConnID, destConnID ConnectionID)
ClosedConnection(CloseReason)
SentTransportParameters(*TransportParameters)
ReceivedTransportParameters(*TransportParameters)
Expand Down
8 changes: 4 additions & 4 deletions logging/mock_connection_tracer_test.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions logging/multiplex.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,9 @@ func NewMultiplexedConnectionTracer(tracers ...ConnectionTracer) ConnectionTrace
return &connTracerMultiplexer{tracers: tracers}
}

func (m *connTracerMultiplexer) StartedConnection(local, remote net.Addr, version VersionNumber, srcConnID, destConnID ConnectionID) {
func (m *connTracerMultiplexer) StartedConnection(local, remote net.Addr, srcConnID, destConnID ConnectionID) {
for _, t := range m.tracers {
t.StartedConnection(local, remote, version, srcConnID, destConnID)
t.StartedConnection(local, remote, srcConnID, destConnID)
}
}

Expand Down
6 changes: 3 additions & 3 deletions logging/multiplex_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,9 @@ var _ = Describe("Tracing", func() {
It("trace the ConnectionStarted event", func() {
local := &net.UDPAddr{IP: net.IPv4(1, 2, 3, 4)}
remote := &net.UDPAddr{IP: net.IPv4(4, 3, 2, 1)}
tr1.EXPECT().StartedConnection(local, remote, VersionNumber(1234), ConnectionID{1, 2, 3, 4}, ConnectionID{4, 3, 2, 1})
tr2.EXPECT().StartedConnection(local, remote, VersionNumber(1234), ConnectionID{1, 2, 3, 4}, ConnectionID{4, 3, 2, 1})
tracer.StartedConnection(local, remote, VersionNumber(1234), ConnectionID{1, 2, 3, 4}, ConnectionID{4, 3, 2, 1})
tr1.EXPECT().StartedConnection(local, remote, ConnectionID{1, 2, 3, 4}, ConnectionID{4, 3, 2, 1})
tr2.EXPECT().StartedConnection(local, remote, ConnectionID{1, 2, 3, 4}, ConnectionID{4, 3, 2, 1})
tracer.StartedConnection(local, remote, ConnectionID{1, 2, 3, 4}, ConnectionID{4, 3, 2, 1})
})

It("traces the ClosedConnection event", func() {
Expand Down
4 changes: 0 additions & 4 deletions qlog/event.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,8 @@ type eventConnectionStarted struct {
SrcAddr *net.UDPAddr
DestAddr *net.UDPAddr

Version protocol.VersionNumber
SrcConnectionID protocol.ConnectionID
DestConnectionID protocol.ConnectionID

// TODO: add ALPN
}

var _ eventDetails = &eventConnectionStarted{}
Expand All @@ -82,7 +79,6 @@ func (e eventConnectionStarted) MarshalJSONObject(enc *gojay.Encoder) {
enc.IntKey("src_port", e.SrcAddr.Port)
enc.StringKey("dst_ip", e.DestAddr.IP.String())
enc.IntKey("dst_port", e.DestAddr.Port)
enc.StringKey("quic_version", versionNumber(e.Version).String())
enc.StringKey("src_cid", connectionID(e.SrcConnectionID).String())
enc.StringKey("dst_cid", connectionID(e.DestConnectionID).String())
}
Expand Down
3 changes: 1 addition & 2 deletions qlog/qlog.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ func (t *connectionTracer) recordEvent(eventTime time.Time, details eventDetails
}
}

func (t *connectionTracer) StartedConnection(local, remote net.Addr, version protocol.VersionNumber, srcConnID, destConnID protocol.ConnectionID) {
func (t *connectionTracer) StartedConnection(local, remote net.Addr, srcConnID, destConnID protocol.ConnectionID) {
// ignore this event if we're not dealing with UDP addresses here
localAddr, ok := local.(*net.UDPAddr)
if !ok {
Expand All @@ -176,7 +176,6 @@ func (t *connectionTracer) StartedConnection(local, remote net.Addr, version pro
t.recordEvent(time.Now(), &eventConnectionStarted{
SrcAddr: localAddr,
DestAddr: remoteAddr,
Version: version,
SrcConnectionID: srcConnID,
DestConnectionID: destConnID,
})
Expand Down
2 changes: 0 additions & 2 deletions qlog/qlog_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,6 @@ var _ = Describe("Tracing", func() {
tracer.StartedConnection(
&net.UDPAddr{IP: net.IPv4(192, 168, 13, 37), Port: 42},
&net.UDPAddr{IP: net.IPv4(192, 168, 12, 34), Port: 24},
0xdeadbeef,
protocol.ConnectionID{1, 2, 3, 4},
protocol.ConnectionID{5, 6, 7, 8},
)
Expand All @@ -166,7 +165,6 @@ var _ = Describe("Tracing", func() {
Expect(ev).To(HaveKeyWithValue("src_port", float64(42)))
Expect(ev).To(HaveKeyWithValue("dst_ip", "192.168.12.34"))
Expect(ev).To(HaveKeyWithValue("dst_port", float64(24)))
Expect(ev).To(HaveKeyWithValue("quic_version", "deadbeef"))
Expect(ev).To(HaveKeyWithValue("src_cid", "01020304"))
Expect(ev).To(HaveKeyWithValue("dst_cid", "05060708"))
})
Expand Down
1 change: 0 additions & 1 deletion session.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1124,7 +1124,6 @@ func (s *session) handleUnpackedPacket(
s.tracer.StartedConnection(
s.conn.LocalAddr(),
s.conn.RemoteAddr(),
s.version,
packet.hdr.SrcConnectionID,
packet.hdr.DestConnectionID,
)
Expand Down
16 changes: 8 additions & 8 deletions session_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -588,7 +588,7 @@ var _ = Describe("Session", func() {
}, nil
})
gomock.InOrder(
tracer.EXPECT().StartedConnection(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()),
tracer.EXPECT().StartedConnection(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()),
tracer.EXPECT().ReceivedPacket(gomock.Any(), gomock.Any(), gomock.Any()),
tracer.EXPECT().ClosedConnection(gomock.Any()),
tracer.EXPECT().Close(),
Expand Down Expand Up @@ -761,7 +761,7 @@ var _ = Describe("Session", func() {
)
sess.receivedPacketHandler = rph
packet.rcvTime = rcvTime
tracer.EXPECT().StartedConnection(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())
tracer.EXPECT().StartedConnection(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())
tracer.EXPECT().ReceivedPacket(hdr, protocol.ByteCount(len(packet.data)), []logging.Frame{})
Expect(sess.handlePacketImpl(packet)).To(BeTrue())
})
Expand Down Expand Up @@ -790,7 +790,7 @@ var _ = Describe("Session", func() {
)
sess.receivedPacketHandler = rph
packet.rcvTime = rcvTime
tracer.EXPECT().StartedConnection(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())
tracer.EXPECT().StartedConnection(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())
tracer.EXPECT().ReceivedPacket(hdr, protocol.ByteCount(len(packet.data)), []logging.Frame{&logging.PingFrame{}})
Expect(sess.handlePacketImpl(packet)).To(BeTrue())
})
Expand Down Expand Up @@ -860,7 +860,7 @@ var _ = Describe("Session", func() {
hdr: &wire.ExtendedHeader{Header: *hdr},
}, nil
}).Times(3)
tracer.EXPECT().StartedConnection(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())
tracer.EXPECT().StartedConnection(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())
tracer.EXPECT().ReceivedPacket(gomock.Any(), gomock.Any(), gomock.Any()).Do(func(hdr *wire.ExtendedHeader, _ protocol.ByteCount, _ []logging.Frame) {
}).Times(3)
packer.EXPECT().PackCoalescedPacket() // only expect a single call
Expand Down Expand Up @@ -905,7 +905,7 @@ var _ = Describe("Session", func() {
hdr: &wire.ExtendedHeader{Header: *hdr},
}, nil
}).Times(3)
tracer.EXPECT().StartedConnection(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())
tracer.EXPECT().StartedConnection(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())
tracer.EXPECT().ReceivedPacket(gomock.Any(), gomock.Any(), gomock.Any()).Do(func(hdr *wire.ExtendedHeader, _ protocol.ByteCount, _ []logging.Frame) {
}).Times(3)
packer.EXPECT().PackCoalescedPacket().Times(3) // only expect a single call
Expand Down Expand Up @@ -1077,7 +1077,7 @@ var _ = Describe("Session", func() {
data: []byte{0}, // one PADDING frame
}, nil)
p1 := getPacket(hdr1, nil)
tracer.EXPECT().StartedConnection(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())
tracer.EXPECT().StartedConnection(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())
tracer.EXPECT().ReceivedPacket(gomock.Any(), protocol.ByteCount(len(p1.data)), gomock.Any())
Expect(sess.handlePacketImpl(p1)).To(BeTrue())
// The next packet has to be ignored, since the source connection ID doesn't match.
Expand Down Expand Up @@ -1119,15 +1119,15 @@ var _ = Describe("Session", func() {
PacketNumberLen: protocol.PacketNumberLen1,
}, nil)
packet.remoteAddr = &net.IPAddr{IP: net.IPv4(192, 168, 0, 100)}
tracer.EXPECT().StartedConnection(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())
tracer.EXPECT().StartedConnection(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())
tracer.EXPECT().ReceivedPacket(gomock.Any(), protocol.ByteCount(len(packet.data)), gomock.Any())
Expect(sess.handlePacketImpl(packet)).To(BeTrue())
})
})

Context("coalesced packets", func() {
BeforeEach(func() {
tracer.EXPECT().StartedConnection(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).MaxTimes(1)
tracer.EXPECT().StartedConnection(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).MaxTimes(1)
})
getPacketWithLength := func(connID protocol.ConnectionID, length protocol.ByteCount) (int /* header length */, *receivedPacket) {
hdr := &wire.ExtendedHeader{
Expand Down

0 comments on commit c30a45e

Please sign in to comment.