DynamoDB events by session ID #13284

smallinsky · 2022-06-08T14:23:08Z

Issue:

Description:

sessionEvents, err := a.alog.GetSessionEvents(namespace, sid, 0, false)

call to

sessionEvents, _, err := a.alog.SearchSessionEvents(time.Time{}, a.authServer.clock.Now().UTC()

The SearchSessionEvents is called with time.Time{} argument (Right now the /webapi/sites/main/sessions/84079b13-29fc-4adf-9563-d3c654d32c5d/events call doesn't provide information about fromTime) that result in hundreds of dynamoDB API call - searchEventsWithFilter will call API for each date within time.Time{} - time.Now() range.

Fix

In case of /webapi/sites/main/sessions/84079b13-29fc-4adf-9563-d3c654d32c5d/events serach session events based on primary sessionID index instead of secondary timesearchV2 global index.

Alternative approaches:

Provide fromTime argument from FE (Cons: fromTime can different from timeNow by 1 year in that case the dynamoDB API will base called 365 times)
Iterate in descending order with an additional stop function that will break the date loop when a session.end or desktop.session.end event was found.

rosstimothy

This seems reasonable to me. My two concerns are:

Why is the sessionID not being propagated in some places?
Is this truly a Dynamo only issue? Does Firestore experience the same sluggishness?

lib/events/multilog.go

lib/events/firestoreevents/firestoreevents.go

rosstimothy · 2022-06-09T13:24:21Z

lib/events/firestoreevents/firestoreevents.go

@@ -548,7 +549,7 @@ func (l *Log) searchEventsOnce(fromUTC, toUTC time.Time, namespace string, limit

 // SearchSessionEvents returns session related events only. This is used to
 // find completed sessions.
-func (l *Log) SearchSessionEvents(fromUTC, toUTC time.Time, limit int, order types.EventOrder, startKey string, cond *types.WhereExpr) ([]apievents.AuditEvent, string, error) {
+func (l *Log) SearchSessionEvents(fromUTC, toUTC time.Time, limit int, order types.EventOrder, startKey string, cond *types.WhereExpr, sessionID string) ([]apievents.AuditEvent, string, error) {


Do we know if Firestore has the same performance issues as Dynamo? Should the sessionID be used here at all if provided?

Firestore does only one call though the sessionID index is not leverage during query. I have extended the query call in case if sessionID is present.

rosstimothy · 2022-06-09T13:28:56Z

lib/events/filelog.go

@@ -362,7 +362,7 @@ func getCheckpointFromEvent(event apievents.AuditEvent) (string, error) {
 	return event.GetID(), nil
 }

-func (l *FileLog) SearchSessionEvents(fromUTC, toUTC time.Time, limit int, order types.EventOrder, startKey string, cond *types.WhereExpr) ([]apievents.AuditEvent, string, error) {
+func (l *FileLog) SearchSessionEvents(fromUTC, toUTC time.Time, limit int, order types.EventOrder, startKey string, cond *types.WhereExpr, sessionID string) ([]apievents.AuditEvent, string, error) {


Would this benefit at all from using the sessionID?

No, The sesisonID was only added here to fulfil common interfaces

I don't think it's necessary for this PR, but from looking at findInFile I think it could potentially benefit from using the sessionID if one was provided.

rosstimothy · 2022-06-09T13:30:35Z

lib/auth/clt.go

@@ -1161,7 +1161,7 @@ func (c *Client) SearchEvents(fromUTC, toUTC time.Time, namespace string, eventT
 }

 // SearchSessionEvents returns session related events to find completed sessions.
-func (c *Client) SearchSessionEvents(fromUTC, toUTC time.Time, limit int, order types.EventOrder, startKey string, cond *types.WhereExpr) ([]apievents.AuditEvent, string, error) {
+func (c *Client) SearchSessionEvents(fromUTC, toUTC time.Time, limit int, order types.EventOrder, startKey string, cond *types.WhereExpr, sessionID string) ([]apievents.AuditEvent, string, error) {
 	events, lastKey, err := c.APIClient.SearchSessionEvents(context.TODO(), fromUTC, toUTC, limit, order, startKey)


Shouldn't the APIClient be updated to take the sessionID?

The only place right now where the ClientI.SearchSessionEvents is called is

teleport/lib/web/apiserver.go

Line 2206 in 9cc58cc

return clt.SearchSessionEvents(from, to, limit, order, startKey, nil)

func (h *Handler) clusterSearchSessionEvents(w http.ResponseWriter, r *http.Request, p httprouter.Params, ctx *SessionContext, site reversetunnel.RemoteSite) (interface{}, error) { searchSessionEvents := func(clt auth.ClientI, from, to time.Time, limit int, order types.EventOrder, startKey string) ([]apievents.AuditEvent, string, error) { return clt.SearchSessionEvents(from, to, limit, order, startKey, nil, "") } return clusterEventsList(ctx, site, r.URL.Query(), searchSessionEvents) }

Where both types.WhereExpr and sesionID are ignored.
The usage of cond *types.WhereExpr and sessionID string is only scoped to internal teleport flow when "where" role filtering param was set.

lib/auth/auth_with_roles.go

rosstimothy · 2022-06-09T16:22:42Z

lib/events/dynamoevents/dynamoevents_test.go

+	c.Assert(err, check.NotNil)
+}
+
+func (s *DynamoeventsSuite) TestSearchSessionEvensBySessionID(c *check.C) {


It's too bad CI doesn't run the DynamoeventsSuite tests 😞

rosstimothy · 2022-06-10T14:03:10Z

lib/events/filelog.go

@@ -362,7 +362,7 @@ func getCheckpointFromEvent(event apievents.AuditEvent) (string, error) {
 	return event.GetID(), nil
 }

-func (l *FileLog) SearchSessionEvents(fromUTC, toUTC time.Time, limit int, order types.EventOrder, startKey string, cond *types.WhereExpr) ([]apievents.AuditEvent, string, error) {
+func (l *FileLog) SearchSessionEvents(fromUTC, toUTC time.Time, limit int, order types.EventOrder, startKey string, cond *types.WhereExpr, sessionID string) ([]apievents.AuditEvent, string, error) {


I don't think it's necessary for this PR, but from looking at findInFile I think it could potentially benefit from using the sessionID if one was provided.

smallinsky · 2022-06-10T15:46:38Z

@xacrimon Could you take a look ?

…ession_events

github-actions · 2022-07-04T11:46:27Z

@smallinsky See the table below for backport results.

Branch	Result
branch/v10	Create PR
branch/v8	Failed
branch/v9	Failed

DynamoDB events by session ID

26b2714

smallinsky force-pushed the smallinsky/search_session_events branch from e0e8bb6 to 26b2714 Compare June 8, 2022 16:34

smallinsky requested review from zmb3 and xacrimon June 8, 2022 16:48

smallinsky marked this pull request as ready for review June 9, 2022 12:27

smallinsky requested a review from rosstimothy June 9, 2022 12:27

github-actions bot requested a review from Joerger June 9, 2022 12:27

github-actions bot added the audit-log Issues related to Teleports Audit Log label Jun 9, 2022

smallinsky removed the request for review from Joerger June 9, 2022 12:57

webvictim linked an issue Jun 9, 2022 that may be closed by this pull request

Replaying Session results in network timeout error #11673

Closed

rosstimothy reviewed Jun 9, 2022

View reviewed changes

kylern added the c-fi Internal Customer Reference label Jun 9, 2022

smallinsky added 2 commits June 10, 2022 13:09

update

e203658

Firestore fix

aa4cc95

smallinsky force-pushed the smallinsky/search_session_events branch from 40737e0 to aa4cc95 Compare June 10, 2022 12:11

rosstimothy approved these changes Jun 10, 2022

View reviewed changes

Add firebase test coverage

f734952

smallinsky force-pushed the smallinsky/search_session_events branch from e4585db to f734952 Compare June 10, 2022 15:50

Make linter happy again

ea129a5

r0mant requested a review from fspmarshall June 13, 2022 15:52

zmb3 added the backport/branch/v9 label Jun 13, 2022

xacrimon approved these changes Jun 22, 2022

View reviewed changes

smallinsky added 3 commits June 22, 2022 16:59

Merge branch 'master' into smallinsky/search_session_events

8767541

Merge remote-tracking branch 'origin/master' into smallinsky/search_s…

1c67224

…ession_events

Merge branch 'master' into smallinsky/search_session_events

bae40a4

smallinsky added the backport/branch/v10 label Jul 4, 2022

smallinsky added the backport/branch/v8 label Jul 4, 2022

smallinsky merged commit eb2401d into master Jul 4, 2022

smallinsky mentioned this pull request Jul 4, 2022

[v10] DynamoDB events by session ID #14077

Merged

webvictim mentioned this pull request Jul 5, 2022

Session playback for a user with where condition set fails #13478

Closed

webvictim linked an issue Jul 6, 2022 that may be closed by this pull request

Session playback for a user with where condition set fails #13478

Closed

smallinsky mentioned this pull request Jul 14, 2022

[v9] DynamoDB events by session ID #14303

Merged

zmb3 deleted the smallinsky/search_session_events branch July 14, 2022 21:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DynamoDB events by session ID #13284

DynamoDB events by session ID #13284

smallinsky commented Jun 8, 2022 •

edited

Loading

rosstimothy left a comment

rosstimothy Jun 9, 2022

smallinsky Jun 10, 2022

rosstimothy Jun 9, 2022

smallinsky Jun 10, 2022

rosstimothy Jun 10, 2022

rosstimothy Jun 9, 2022

smallinsky Jun 10, 2022

rosstimothy Jun 9, 2022

rosstimothy Jun 10, 2022

smallinsky commented Jun 10, 2022

github-actions bot commented Jul 4, 2022

DynamoDB events by session ID #13284

DynamoDB events by session ID #13284

Conversation

smallinsky commented Jun 8, 2022 • edited Loading

Issue:

Description:

Fix

Alternative approaches:

rosstimothy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smallinsky commented Jun 10, 2022

github-actions bot commented Jul 4, 2022

smallinsky commented Jun 8, 2022 •

edited

Loading