From 5c43bf12c8440cbce67181aa132dfbc9f699c7ea Mon Sep 17 00:00:00 2001 From: Andy Keller Date: Mon, 14 Mar 2022 14:57:11 -0400 Subject: [PATCH 1/5] Expose the RemoteAddr() and request Header() via the Connection interface --- server/connection.go | 11 +++++++++++ server/serverimpl.go | 8 ++++---- server/types/connection.go | 8 ++++++++ 3 files changed, 23 insertions(+), 4 deletions(-) diff --git a/server/connection.go b/server/connection.go index 64947ba5..40230a24 100644 --- a/server/connection.go +++ b/server/connection.go @@ -2,6 +2,8 @@ package server import ( "context" + "net" + "net/http" "github.com/gorilla/websocket" "google.golang.org/protobuf/proto" @@ -12,10 +14,19 @@ import ( type connection struct { wsConn *websocket.Conn + header http.Header } var _ types.Connection = (*connection)(nil) +func (c connection) RemoteAddr() net.Addr { + return c.RemoteAddr() +} + +func (c connection) Header() http.Header { + return c.header +} + func (c connection) Send(ctx context.Context, message *protobufs.ServerToAgent) error { bytes, err := proto.Marshal(message) if err != nil { diff --git a/server/serverimpl.go b/server/serverimpl.go index dc34bcac..293bbfb7 100644 --- a/server/serverimpl.go +++ b/server/serverimpl.go @@ -151,12 +151,12 @@ func (s *server) httpHandler(w http.ResponseWriter, req *http.Request) { } // Return from this func to reduce memory usage. - // Handle the connection on a separate gorountine. - go s.handleWSConnection(conn) + // Handle the connection on a separate goroutine. + go s.handleWSConnection(conn, req.Header) } -func (s *server) handleWSConnection(wsConn *websocket.Conn) { - agentConn := connection{wsConn: wsConn} +func (s *server) handleWSConnection(wsConn *websocket.Conn, reqHeader http.Header) { + agentConn := connection{wsConn: wsConn, header: reqHeader} defer func() { // Close the connection when all is done. diff --git a/server/types/connection.go b/server/types/connection.go index fe706be3..35497d61 100644 --- a/server/types/connection.go +++ b/server/types/connection.go @@ -2,6 +2,8 @@ package types import ( "context" + "net" + "net/http" "github.com/open-telemetry/opamp-go/protobufs" ) @@ -9,6 +11,12 @@ import ( // Connection represents one OpAMP WebSocket connections. // The implementation MUST be a comparable type so that it can be used as a map key. type Connection interface { + // RemoteAddr returns the remote network address of the connection. + RemoteAddr() net.Addr + + // Header returns the request header fields of the http request that opened the connection. + Header() http.Header + // Send a message. Should not be called concurrently for the same Connection instance. // Blocks until the message is sent. // Should return as soon as possible if the ctx is cancelled. From 967d5dfba3fdd09c04b6289b6a59f141c2e0469a Mon Sep 17 00:00:00 2001 From: Andy Keller Date: Tue, 22 Mar 2022 13:48:34 -0400 Subject: [PATCH 2/5] Remove Header() for now per PR discussion --- server/connection.go | 6 ------ server/serverimpl.go | 6 +++--- server/types/connection.go | 4 ---- 3 files changed, 3 insertions(+), 13 deletions(-) diff --git a/server/connection.go b/server/connection.go index 40230a24..8a576c0a 100644 --- a/server/connection.go +++ b/server/connection.go @@ -3,7 +3,6 @@ package server import ( "context" "net" - "net/http" "github.com/gorilla/websocket" "google.golang.org/protobuf/proto" @@ -14,7 +13,6 @@ import ( type connection struct { wsConn *websocket.Conn - header http.Header } var _ types.Connection = (*connection)(nil) @@ -23,10 +21,6 @@ func (c connection) RemoteAddr() net.Addr { return c.RemoteAddr() } -func (c connection) Header() http.Header { - return c.header -} - func (c connection) Send(ctx context.Context, message *protobufs.ServerToAgent) error { bytes, err := proto.Marshal(message) if err != nil { diff --git a/server/serverimpl.go b/server/serverimpl.go index 293bbfb7..c2c6eb67 100644 --- a/server/serverimpl.go +++ b/server/serverimpl.go @@ -152,11 +152,11 @@ func (s *server) httpHandler(w http.ResponseWriter, req *http.Request) { // Return from this func to reduce memory usage. // Handle the connection on a separate goroutine. - go s.handleWSConnection(conn, req.Header) + go s.handleWSConnection(conn) } -func (s *server) handleWSConnection(wsConn *websocket.Conn, reqHeader http.Header) { - agentConn := connection{wsConn: wsConn, header: reqHeader} +func (s *server) handleWSConnection(wsConn *websocket.Conn) { + agentConn := connection{wsConn: wsConn} defer func() { // Close the connection when all is done. diff --git a/server/types/connection.go b/server/types/connection.go index 35497d61..26da474d 100644 --- a/server/types/connection.go +++ b/server/types/connection.go @@ -3,7 +3,6 @@ package types import ( "context" "net" - "net/http" "github.com/open-telemetry/opamp-go/protobufs" ) @@ -14,9 +13,6 @@ type Connection interface { // RemoteAddr returns the remote network address of the connection. RemoteAddr() net.Addr - // Header returns the request header fields of the http request that opened the connection. - Header() http.Header - // Send a message. Should not be called concurrently for the same Connection instance. // Blocks until the message is sent. // Should return as soon as possible if the ctx is cancelled. From dea2bd00cdae06fe8c3786f3a3c58e54b9da3092 Mon Sep 17 00:00:00 2001 From: Andy Keller Date: Tue, 5 Apr 2022 09:22:05 -0600 Subject: [PATCH 3/5] Get RemoteAddr from the websocket --- server/connection.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/connection.go b/server/connection.go index 8a576c0a..9f62c9ec 100644 --- a/server/connection.go +++ b/server/connection.go @@ -18,7 +18,7 @@ type connection struct { var _ types.Connection = (*connection)(nil) func (c connection) RemoteAddr() net.Addr { - return c.RemoteAddr() + return c.wsConn.RemoteAddr() } func (c connection) Send(ctx context.Context, message *protobufs.ServerToAgent) error { From 8fde9bb6a579b5023a19b95a25a9434d17f2ce2e Mon Sep 17 00:00:00 2001 From: Andy Keller Date: Tue, 5 Apr 2022 09:34:45 -0600 Subject: [PATCH 4/5] Verify RemoteAddr() in TestServerStartAcceptConnection --- server/serverimpl_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/server/serverimpl_test.go b/server/serverimpl_test.go index 6fabec35..0499f6fd 100644 --- a/server/serverimpl_test.go +++ b/server/serverimpl_test.go @@ -114,6 +114,9 @@ func TestServerStartAcceptConnection(t *testing.T) { eventually(t, func() bool { return atomic.LoadInt32(&connectedCalled) == 1 }) assert.True(t, atomic.LoadInt32(&connectionCloseCalled) == 0) + // Verify that the RemoteAddr is correct. + require.Equal(t, conn.LocalAddr().String(), srvConn.RemoteAddr().String()) + // Close the connection from client side. conn.Close() From 49c6526c833938c733f75c963711e491172c90d0 Mon Sep 17 00:00:00 2001 From: Andy Keller Date: Tue, 5 Apr 2022 09:38:10 -0600 Subject: [PATCH 5/5] save connection before setting connectedCalled to avoid race --- server/serverimpl_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/serverimpl_test.go b/server/serverimpl_test.go index 0499f6fd..b0da2487 100644 --- a/server/serverimpl_test.go +++ b/server/serverimpl_test.go @@ -89,8 +89,8 @@ func TestServerStartAcceptConnection(t *testing.T) { return types.ConnectionResponse{Accept: true} }, OnConnectedFunc: func(conn types.Connection) { - atomic.StoreInt32(&connectedCalled, 1) srvConn = conn + atomic.StoreInt32(&connectedCalled, 1) }, OnConnectionCloseFunc: func(conn types.Connection) { atomic.StoreInt32(&connectionCloseCalled, 1)