Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

web: stream recorded SSH/Kube sessions rather than downloading them #36168

Merged
merged 1 commit into from
Jan 6, 2024

Conversation

zmb3
Copy link
Collaborator

@zmb3 zmb3 commented Jan 1, 2024

Prior to this, the web UI would download the entire session recording
and store it in JavaScript memory before starting playback. This caused
the browser tab to crash when attempting to play back sessions larger
than ~5MB.

Updates https://github.com/gravitational/teleport-private/issues/1024
Closes https://github.com/gravitational/teleport-private/issues/665
Closes #10578

@zmb3 zmb3 requested a review from Tener January 1, 2024 20:25
@github-actions github-actions bot requested review from avatus and rudream January 1, 2024 20:25
Copy link

github-actions bot commented Jan 1, 2024

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

@@ -101,6 +101,8 @@ export async function resolveServerMessage(
}
}

// TODO(zmb3): check with Ryan about replacing this with streaming
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @ryanclark how critical is getSessionEvents / getSessionStream given the de-investment in assist?

I'd really like to remove these APIs from the frontend for v15 so that I can remove them from the backend in v16.

@zmb3 zmb3 force-pushed the zmb3/web-stream branch from 3b98744 to 8d21e02 Compare January 1, 2024 20:43
Copy link

github-actions bot commented Jan 1, 2024

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

lib/web/tty_playback.go Outdated Show resolved Hide resolved
Copy link
Contributor

@avatus avatus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Frontend looks good to me

web/packages/teleport/src/Player/Player.tsx Outdated Show resolved Hide resolved
web/packages/teleport/src/Player/SshPlayer.tsx Outdated Show resolved Hide resolved
web/packages/teleport/src/Player/SshPlayer.tsx Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Jan 2, 2024

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

@ravicious ravicious self-requested a review January 3, 2024 09:23
Copy link

github-actions bot commented Jan 3, 2024

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

Copy link
Contributor

@Tener Tener left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To ensure the fuzz tests are actually executed, you need to add them to fuzz/oss-fuzz-build.sh. @jentfoo we should have an automated test checking this, is this anywhere on a radar?

@jentfoo
Copy link
Contributor

jentfoo commented Jan 4, 2024

we should have an automated test checking this, is this anywhere on a radar?

A couple months ago I went and reviewed all of our fuzzing, ensured good seeds and coverage as well as oss-fuzz. So I think we are generally in a good place. However after that effort I did have to remove several tests from oss-fuzz due to a bug on their side resulting in timeout failures that were unable to be reproduced outside of oss-fuzz.

Long story short, we should add this to oss-fuzz, but we can not ensure all fuzzing is done within oss-fuzz due to the timeout false positives. And there is not currently any automation to validate what is in oss-fuzz or not.

@ravicious ravicious removed their request for review January 4, 2024 14:52
Copy link

github-actions bot commented Jan 5, 2024

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

1 similar comment
Copy link

github-actions bot commented Jan 6, 2024

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

@zmb3 zmb3 added the no-changelog Indicates that a PR does not require a changelog entry label Jan 6, 2024
Prior to this, the web UI would download the entire session recording
and store it in JavaScript memory before starting playback. This caused
the browser tab to crash when attempting to play back sessions larger
than ~5MB.

For playback, we use a custom binary protocol rather than the protobuf
envelopes that we use for live sessions. The protobuf envelopes only
send raw PTY data, there is no place to put the timing data. Adding
fields to the envelope would be a disruptive change because our JS
codec is hand-rolled and we'd have to make the parsing updates manually.

Updates gravitational/teleport-private#1024
Closes gravitational/teleport-private#665
Closes #10578
@zmb3 zmb3 force-pushed the zmb3/web-stream branch from f5a8ecd to 755dec6 Compare January 6, 2024 18:06
@zmb3 zmb3 enabled auto-merge January 6, 2024 18:06
@zmb3 zmb3 added this pull request to the merge queue Jan 6, 2024
Merged via the queue into master with commit 6dad93c Jan 6, 2024
39 checks passed
@zmb3 zmb3 deleted the zmb3/web-stream branch January 6, 2024 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Indicates that a PR does not require a changelog entry size/sm ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Playback of long (~200 MB) session crashes the browser tab
4 participants