From 32070678dc712812a42a511bb68f668658bd327a Mon Sep 17 00:00:00 2001 From: Timothy Messier Date: Mon, 23 May 2022 18:27:14 +0000 Subject: [PATCH] perf(session): Remove connections from session list endpoint Including the nested connection information in the session list response had a negative impact on performance as it created a significant multiplier on the number of rows that needed to be fetched from the database. It was also determined that this information was used by neither the admin ui nor the desktop client. Note that nested connection information can still be retrieved via a read request for a specific session. --- .../handlers/sessions/session_service_test.go | 30 +++------------ .../postgres/20/09_session_list_view.up.sql | 1 + .../31/02_session_list_no_connections.up.sql | 38 +++++++++++++++++++ internal/session/repository.go | 25 ------------ internal/session/repository_session_test.go | 3 +- internal/session/session.go | 10 ----- 6 files changed, 46 insertions(+), 61 deletions(-) create mode 100644 internal/db/schema/migrations/oss/postgres/31/02_session_list_no_connections.up.sql diff --git a/internal/daemon/controller/handlers/sessions/session_service_test.go b/internal/daemon/controller/handlers/sessions/session_service_test.go index 2b3d7b7a6a..d18499c4f3 100644 --- a/internal/daemon/controller/handlers/sessions/session_service_test.go +++ b/internal/daemon/controller/handlers/sessions/session_service_test.go @@ -281,7 +281,7 @@ func TestList(t *testing.T) { Endpoint: "tcp://127.0.0.1:22", }) - c := session.TestConnection(t, conn, sess.PublicId, "127.0.0.1", 22, "127.0.0.2", 23, "127.0.0.1") + session.TestConnection(t, conn, sess.PublicId, "127.0.0.1", 22, "127.0.0.2", 23, "127.0.0.1") status, states := convertStates(sess.States) @@ -304,14 +304,7 @@ func TestList(t *testing.T) { Certificate: sess.Certificate, Type: tcp.Subtype.String(), AuthorizedActions: testAuthorizedActions, - Connections: []*pb.Connection{ - { - ClientTcpAddress: c.ClientTcpAddress, - ClientTcpPort: c.ClientTcpPort, - EndpointTcpAddress: c.EndpointTcpAddress, - EndpointTcpPort: c.EndpointTcpPort, - }, - }, + Connections: []*pb.Connection{}, // connections should not be returned for list }) totalSession = append(totalSession, wantSession[i]) @@ -326,7 +319,7 @@ func TestList(t *testing.T) { Endpoint: "tcp://127.0.0.1:22", }) - c = session.TestConnection(t, conn, sess.PublicId, "127.0.0.1", 22, "127.0.0.2", 23, "127.0.0.1") + session.TestConnection(t, conn, sess.PublicId, "127.0.0.1", 22, "127.0.0.2", 23, "127.0.0.1") status, states = convertStates(sess.States) @@ -349,14 +342,7 @@ func TestList(t *testing.T) { Certificate: sess.Certificate, Type: tcp.Subtype.String(), AuthorizedActions: testAuthorizedActions, - Connections: []*pb.Connection{ - { - ClientTcpAddress: c.ClientTcpAddress, - ClientTcpPort: c.ClientTcpPort, - EndpointTcpAddress: c.EndpointTcpAddress, - EndpointTcpPort: c.EndpointTcpPort, - }, - }, + Connections: []*pb.Connection{}, // connections should not be returned for list }) } @@ -423,14 +409,8 @@ func TestList(t *testing.T) { require.Equal(len(tc.res.GetItems()), len(got.GetItems()), "Didn't get expected number of sessions: %v", got.GetItems()) for i, wantSess := range tc.res.GetItems() { assert.True(got.GetItems()[i].GetExpirationTime().AsTime().Sub(wantSess.GetExpirationTime().AsTime()) < 10*time.Millisecond) - assert.Equal(1, len(wantSess.GetConnections())) + assert.Equal(0, len(wantSess.GetConnections())) // no connections on list wantSess.ExpirationTime = got.GetItems()[i].GetExpirationTime() - for _, c := range got.GetItems()[i].GetConnections() { - assert.Equal("127.0.0.1", c.ClientTcpAddress) - assert.Equal(uint32(22), c.ClientTcpPort) - assert.Equal("127.0.0.2", c.EndpointTcpAddress) - assert.Equal(uint32(23), c.EndpointTcpPort) - } } } assert.Empty(cmp.Diff(got, tc.res, protocmp.Transform()), "ListSessions(%q) got response %q, wanted %q", tc.req, got, tc.res) diff --git a/internal/db/schema/migrations/oss/postgres/20/09_session_list_view.up.sql b/internal/db/schema/migrations/oss/postgres/20/09_session_list_view.up.sql index 2feb2a68cc..8d9b9f30fd 100644 --- a/internal/db/schema/migrations/oss/postgres/20/09_session_list_view.up.sql +++ b/internal/db/schema/migrations/oss/postgres/20/09_session_list_view.up.sql @@ -1,6 +1,7 @@ begin; -- Replaces the view created in 1/01 to include connections +-- Replaced in 31/0_session_list_no_connections drop view session_with_state; create view session_list as select diff --git a/internal/db/schema/migrations/oss/postgres/31/02_session_list_no_connections.up.sql b/internal/db/schema/migrations/oss/postgres/31/02_session_list_no_connections.up.sql new file mode 100644 index 0000000000..e012f2d773 --- /dev/null +++ b/internal/db/schema/migrations/oss/postgres/31/02_session_list_no_connections.up.sql @@ -0,0 +1,38 @@ +begin; + +-- Replaces the view created in 2/09_session_list_view +drop view session_list; +create view session_list as + select + s.public_id, + s.user_id, + s.host_id, + s.server_id, + s.server_type, + s.target_id, + s.host_set_id, + s.auth_token_id, + s.scope_id, + s.certificate, + s.expiration_time, + s.connection_limit, + s.tofu_token, + s.key_id, + s.termination_reason, + s.version, + s.create_time, + s.update_time, + s.endpoint, + s.worker_filter, + ss.state, + ss.previous_end_time, + ss.start_time, + ss.end_time + from + session s + join + session_state ss + on + s.public_id = ss.session_id; + +commit; diff --git a/internal/session/repository.go b/internal/session/repository.go index 8735c8d7fa..d9f4c860ab 100644 --- a/internal/session/repository.go +++ b/internal/session/repository.go @@ -83,8 +83,6 @@ func (r *Repository) convertToSessions(ctx context.Context, sessionList []*sessi return nil, nil } sessions := []*Session{} - // deduplication map of connections by connection_id - connections := map[string]*Connection{} // deduplication map of states by end_time states := map[*timestamp.Timestamp]*State{} var prevSessionId string @@ -92,10 +90,6 @@ func (r *Repository) convertToSessions(ctx context.Context, sessionList []*sessi for _, sv := range sessionList { if sv.PublicId != prevSessionId { if prevSessionId != "" { - for _, c := range connections { - workingSession.Connections = append(workingSession.Connections, c) - } - connections = map[string]*Connection{} for _, s := range states { workingSession.States = append(workingSession.States, s) } @@ -157,22 +151,6 @@ func (r *Repository) convertToSessions(ctx context.Context, sessionList []*sessi } } - if sv.ConnectionId != "" { - if _, ok := connections[sv.ConnectionId]; !ok { - connections[sv.ConnectionId] = &Connection{ - PublicId: sv.ConnectionId, - SessionId: sv.PublicId, - ClientTcpAddress: sv.ClientTcpAddress, - ClientTcpPort: sv.ClientTcpPort, - EndpointTcpAddress: sv.EndpointTcpAddress, - EndpointTcpPort: sv.EndpointTcpPort, - BytesUp: sv.BytesUp, - BytesDown: sv.BytesDown, - ClosedReason: sv.ClosedReason, - } - } - } - } for _, s := range states { workingSession.States = append(workingSession.States, s) @@ -180,9 +158,6 @@ func (r *Repository) convertToSessions(ctx context.Context, sessionList []*sessi sort.Slice(workingSession.States, func(i, j int) bool { return workingSession.States[i].StartTime.GetTimestamp().AsTime().After(workingSession.States[j].StartTime.GetTimestamp().AsTime()) }) - for _, c := range connections { - workingSession.Connections = append(workingSession.Connections, c) - } sessions = append(sessions, workingSession) return sessions, nil } diff --git a/internal/session/repository_session_test.go b/internal/session/repository_session_test.go index 891354b6b3..acf9801c0d 100644 --- a/internal/session/repository_session_test.go +++ b/internal/session/repository_session_test.go @@ -126,7 +126,8 @@ func TestRepository_ListSession(t *testing.T) { require.NoError(err) assert.Equal(tt.wantCnt, len(got)) for i := 0; i < len(got); i++ { - assert.Equal(tt.withConnections, len(got[i].Connections)) + // connections should not be returned for list requests + assert.Equal(0, len(got[i].Connections)) for _, c := range got[i].Connections { assert.Equal("127.0.0.1", c.ClientTcpAddress) assert.Equal(uint32(22), c.ClientTcpPort) diff --git a/internal/session/session.go b/internal/session/session.go index 7026656e73..5d6faf74ac 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -472,16 +472,6 @@ type sessionListView struct { PreviousEndTime *timestamp.Timestamp `json:"previous_end_time,omitempty" gorm:"default:current_timestamp"` StartTime *timestamp.Timestamp `json:"start_time,omitempty" gorm:"default:current_timestamp;primary_key"` EndTime *timestamp.Timestamp `json:"end_time,omitempty" gorm:"default:current_timestamp"` - - // Connection fields - ConnectionId string `json:"connection_id,omitempty" gorm:"default:null"` - ClientTcpAddress string `json:"client_tcp_address,omitempty" gorm:"default:null"` - ClientTcpPort uint32 `json:"client_tcp_port,omitempty" gorm:"default:null"` - EndpointTcpAddress string `json:"endpoint_tcp_address,omitempty" gorm:"default:null"` - EndpointTcpPort uint32 `json:"endpoint_tcp_port,omitempty" gorm:"default:null"` - BytesUp uint64 `json:"bytes_up,omitempty" gorm:"default:null"` - BytesDown uint64 `json:"bytes_down,omitempty" gorm:"default:null"` - ClosedReason string `json:"closed_reason,omitempty" gorm:"default:null"` } // TableName returns the tablename to override the default gorm table name