Add a retry mechanism to UploadCompleter's session.end checker
#41113
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tigrato
force-pushed
the
tigrato/fix-event-completer
branch
2 times, most recently
from
a157a93 to
2171368
Compare
When uploading session assets, `UploadCompleter` checks if the stale upload contains the `session.end` event. This check is done using `StreamSessionEvents` call to Auth Server to retrieve all the events recorded for a particular session. When the session misses the `session.end` event, the upload completer fakes it and creates it in order to Teleport being able to display the session recordings. Teleport requires `session.end` to be able to display them correctly. The upload completer after finalyzing the upload, schedules a goroutine to run 2min after to properly range all events in the session and creating the event if missed. The issue happens if the upload completer can successfuly complete the upload - auth is operational and s3 storage is also operationa -l but audit events backend isn't available at the time. In those cases, although the recording was correctly completed, emiting a `session.end` event will fail and no-one will be able to retrieve or analyze the session recordings content. This PR adds a logic to keep ensuring the session properly has an end event until the audit log backend is operational again. Signed-off-by: Tiago Silva <[email protected]>
tigrato
force-pushed
the
tigrato/fix-event-completer
branch
from
2171368 to
6498d6f
Compare
zmb3
reviewed
May 15, 2024
| @@ -315,7 +334,24 @@ func (u *UploadCompleter) CheckUploads(ctx context.Context) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (u *UploadCompleter) ensureSessionEndEvent(ctx context.Context, uploadData UploadMetadata) error { | |||
| func (u *UploadCompleter) ensureAllPendingsessionsHaveEndEvts(ctx context.Context) { | |||
There was a problem hiding this comment.
Suggested change
| func (u *UploadCompleter) ensureAllPendingsessionsHaveEndEvts(ctx context.Context) { | |
| func (u *UploadCompleter) ensureAllPendingSessionsHaveEndEvents(ctx context.Context) { |
| @@ -416,3 +452,38 @@ loop: | |||
| } | |||
| return nil | |||
| } | |||
|
|
|||
| func (u *UploadCompleter) createIncompleteSession(sessionID session.ID) error { | |||
| path := filepath.Join(u.cfg.UnconfirmedSessionEndDir, sessionID.String()) | |||
There was a problem hiding this comment.
I see, so we're creating an empty file on disk named after the session ID as in indicator to go back and retry the session.
But then when we retry we have to stream the entire session again to compute what the end event should look like.
Why don't we just write the JSON-encoded end event to this file, so subsequent retries can emit the end event directly rather than having to compute it again?
There was a problem hiding this comment.
Because we don't have the event every time.
If the audit log backend is suffering a temporary failure, streaming can fail and we can't even validate if the session already has the session.end event.
Saying that, we need to retry later to:
- pull all session events and check whether we have a
session.endor not - if audit log failed, create the file and retry later.
- if session doesn't have the
session.endevent, try creating it. If create audit event operation fails we retry later. For this step we can implement the optimization you mentioned but we still need to deal with step 2.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When uploading session assets,
UploadCompleterchecks if the stale upload contains thesession.endevent. This check is done usingStreamSessionEventscall to Auth Server to retrieve all the events recorded for a particular session. When the session misses thesession.endevent, the upload completer fakes it and creates it in order to Teleport being able to display the session recordings. Teleport requiressession.endto be able to display them correctly.The upload completer after finalyzing the upload, schedules a goroutine to run 2min after to properly range all events in the session and creating the event if missed.
The issue happens if the upload completer can successfuly complete the upload - auth is operational and s3 storage is also operationa -l but audit events backend isn't available at the time. In those cases, although the recording was correctly completed, emiting a
session.endevent will fail and no-one will be able to retrieve or analyze the session recordings content.This PR adds a logic to keep ensuring the session properly has an end event until the audit log backend is operational again.