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

Re-add grace period to Upload completer for backwards compatibility. #12471

Merged

Conversation

Joerger
Copy link
Contributor

@Joerger Joerger commented May 5, 2022

Fixes backwards compatibility issues that came from #11551

Specifically, If a node starts a session recording in proxy/sync modes (which records the session directly on the Proxy/Auth servers), then the upload completer in the Proxy/Auth services will periodically check for a corresponding SessionTracker for any uncompleted uploads on disk.

Since pre-v9 sessions do not create session trackers for ongoing sessions, the Proxy/Auth servers will incorrectly assume that their uploads are abandoned. As a result, the upload will be completed prematurely and result in an error in the session due to the audit writer being closed on Proxy/Auth side.

Changes:

  • Re-add grace period to Upload completer for backwards compatibility.
  • Provide grace period to upload completer in Proxy and Auth services so that proxy/sync recording works with old nodes.
  • Further decouple uploader from upload completer so that they can be configured as standalone processes more easily.

TODO: manually test and fix any failing tests

Provide grace period to upload completer in Proxy and Auth services so that proxy/sync recording works with old nodes.

Further decouple uploader from upload completer so that they can be
configured as standalone processes more easily.
@github-actions github-actions bot added the audit-log Issues related to Teleports Audit Log label May 5, 2022
@ibeckermayer
Copy link
Contributor

@zmb3 do we have a similar thing in windows_desktop_service? It seems all the other major feature services are changed here but ours.

@zmb3
Copy link
Collaborator

zmb3 commented May 7, 2022

@zmb3 do we have a similar thing in windows_desktop_service? It seems all the other major feature services are changed here but ours.

You know now that you say this, we might a long-standing bug that the upload completer will not run if you run a standalone desktop service!

Copy link
Contributor

@ibeckermayer ibeckermayer left a comment

Choose a reason for hiding this comment

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

Straining to wrap my head around an unfamiliar part of the code and so could well be wrong, but may have found a logic bug in the change to checkUploads.

lib/events/complete.go Show resolved Hide resolved
Copy link
Contributor

@probakowski probakowski left a comment

Choose a reason for hiding this comment

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

LGTM

@Joerger Joerger enabled auto-merge (squash) May 9, 2022 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit-log Issues related to Teleports Audit Log backport-required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants