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.

Sign up for GitHub

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

Jump to bottom

desktop access: missing upload completer in standalone mode #12549

Closed
zmb3 opened this issue May 10, 2022 · 0 comments · Fixed by #14521
Closed

desktop access: missing upload completer in standalone mode #12549

zmb3 opened this issue May 10, 2022 · 0 comments · Fixed by #14521
Assignees
@zmb3
Labels
audit-log Issues related to Teleports Audit Log bug desktop-access

Comments

@zmb3
Copy link
Collaborator

zmb3 commented May 10, 2022 •
edited
Loading

As pointed out in #12471 (comment), desktop access does not start an upload completer service.

Working theory is that we never noticed this when running windows desktop service + auth service in the same process, because auth service sets up an upload completer.

We should:

  1. Confirm whether this is an issue. To test, run a teleport process with only windows_desktop_service enabled, and set session recording mode to proxy or node. Kill the process running windows_desktop_service while a session is in progress, restart the process, and verify whether the upload is ever completed (you may need to tweak the grace period to make this easier to test).
  2. If there is in fact an issue, fix it. The simplest option would be to mimic the other services, and always start an upload completer. This is not ideal, because we can still end up with 3-5 uploaders on the same system fighting to upload files in the same directory. A better fix would be to add some mechanism to *TeleportProcess to ensure that it always starts an upload completer in node or proxy recording modes, centralizing this logic and moving it out of individual services.

cc @Joerger who has the most recent experience in this code and can help discuss.
@zmb3 zmb3 added bug desktop-access audit-log Issues related to Teleports Audit Log labels May 10, 2022
@zmb3 zmb3 assigned xacrimon and unassigned xacrimon Jul 13, 2022
zmb3 added a commit that referenced this issue Jul 15, 2022
@zmb3
Run a single uploader service per process
a6a42c1 
Prior to this change, each individual service (proxy, app, SSH, db, etc)
would spin up its own uploader service. If you are running multiple
Teleport services in the same process, this means you get multiple
uploaders all looking at the same directory, which can result in
duplicate upload events in the audit log.

Additionally, desktop access has (mistakenly) failed to set up this
service, so desktop sessions would only be uploaded if you happened
to also run some other service in the same process that does spin up
the uploader.

Lastly, the uploader is not necessary at all if sync-recording modes
are used (nothing is written to disk).

Solve these issues by centralizing the uploader service so that it
runs once per process, and each Teleport service doesn't need to think
about whether or not the service should run.

Fixes #12549
@zmb3 zmb3 mentioned this issue Jul 15, 2022
Merged
zmb3 added a commit that referenced this issue Jul 22, 2022
@zmb3
Run a single uploader service per process
5b8f459 
Prior to this change, each individual service (proxy, app, SSH, db, etc)
would spin up its own uploader service. If you are running multiple
Teleport services in the same process, this means you get multiple
uploaders all looking at the same directory, which can result in
duplicate upload events in the audit log.

Additionally, desktop access has (mistakenly) failed to set up this
service, so desktop sessions would only be uploaded if you happened
to also run some other service in the same process that does spin up
the uploader.

Solve these issues by centralizing the uploader service so that it
runs once per process, and each Teleport service doesn't need to think
about whether or not the service should run.

Fixes #12549
@zmb3 zmb3 self-assigned this Aug 8, 2022
zmb3 added a commit that referenced this issue Sep 22, 2022
@zmb3
Run a single uploader service per process
dcd810f 
Prior to this change, each individual service (proxy, app, SSH, db, etc)
would spin up its own uploader service. If you are running multiple
Teleport services in the same process, this means you get multiple
uploaders all looking at the same directory, which can result in
duplicate upload events in the audit log.

Additionally, desktop access has (mistakenly) failed to set up this
service, so desktop sessions would only be uploaded if you happened
to also run some other service in the same process that does spin up
the uploader.

Solve these issues by centralizing the uploader service so that it
runs once per process, and each Teleport service doesn't need to think
about whether or not the service should run.

Fixes #12549
@zmb3 zmb3 mentioned this issue Sep 23, 2022
Merged
zmb3 added a commit that referenced this issue Sep 23, 2022
@zmb3
Init an uploader service for Windows Desktop Service
c8d4cce 
This will result in multiple uploader services running if there are
other Teleport services running in the same process, but that will
be fixed by #14521.

Updates #12549
ryanclark pushed a commit that referenced this issue Sep 23, 2022
@zmb3 @ryanclark
Init an uploader service for Windows Desktop Service
229b587 
This will result in multiple uploader services running if there are
other Teleport services running in the same process, but that will
be fixed by #14521.

Updates #12549
zmb3 added a commit that referenced this issue Sep 23, 2022
@zmb3
Init an uploader service for Windows Desktop Service
cbd5fab 
This will result in multiple uploader services running if there are
other Teleport services running in the same process, but that will
be fixed by #14521.

Updates #12549
r0mant added a commit that referenced this issue Sep 24, 2022
@zmb3 @r0mant
@renatolabs @rosstimothy @hugoShaka @ryanclark @nklaassen
[v10] Misc Backports (#16674)
515824c 
* Init an uploader service for Windows Desktop Service

This will result in multiple uploader services running if there are
other Teleport services running in the same process, but that will
be fixed by #14521.

Updates #12549

* buddy: Fix incorrect use of loop variables (#16306)

* Fix incorrect use of loop variables

This commit fixes a few occurrences of loop variables being
incorrectly used in the context of Go-routines or (most frequently)
parallel tests. To fix the issues, we create a local copy of the range
variables before the parallel tests (or Go-routine), as suggested in
the documentation of the `testing` package:

https://pkg.go.dev/testing#hdr-Subtests_and_Sub_benchmarks

Issues were found using the `loopvarcapture` linter.

Signed-off-by: Roman Tkachenko <[email protected]>

* fix TestTraceProvider/spans_exported_with_gRPC+TLS

* run TestSSH serially

* operator: Conserve 'created_by' data in user spec

Signed-off-by: Roman Tkachenko <[email protected]>
Co-authored-by: Renato Costa <[email protected]>
Co-authored-by: Tim Ross <[email protected]>
Co-authored-by: Hugo Hervieux <[email protected]>

* Correctly set the end time when completing an upload

The upload completer would set the session end time based on the last
known event in the session, but it left the timestamp in the event's
metadata blank, which caused it to default to the time the upload was
completed.

Fixes #16555

* Speed up TestTerminalPing (#16481)

This test takes between 10-20 seconds to run, because it waits for
a websocket ping message and pings run on a 10s timer by default.

It turns out, we allow specifying the keepalive interval in the
terminal request, but due to a bug we would always overwrite that
value with what's in the cluster networking config.

Fix the bug and speed up the test by overriding the keepalive interval
to something shorter for this test.

Before:

    ➜ go test -run TestTerminalPing -race ./lib/web -count=1
    ok      github.com/gravitational/teleport/lib/web       13.122s

After:

    ➜ go test -run TestTerminalPing -race ./lib/web -count=1
    ok      github.com/gravitational/teleport/lib/web       3.394s

* Add labels to Windows Desktop Service, add endpoint for searching them (#16436)

* export GetResourcesByResourceIDs

* add WithCluserClientProvider

* Improve unit tests in CI (#16698)

- Increase timeout
- Detect whether operator, CI, Rust, or Helm tests are necessary

* Update `webassets` ref to latest `teleport-v10`

Signed-off-by: Roman Tkachenko <[email protected]>
Co-authored-by: Roman Tkachenko <[email protected]>
Co-authored-by: Renato Costa <[email protected]>
Co-authored-by: Tim Ross <[email protected]>
Co-authored-by: Hugo Hervieux <[email protected]>
Co-authored-by: Ryan Clark <[email protected]>
Co-authored-by: Nic Klaassen <[email protected]>
Co-authored-by: Ryan Clark <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@zmb3 zmb3

Labels
audit-log Issues related to Teleports Audit Log bug desktop-access
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Run a single uploader service per process gravitational/teleport
2 participants
@zmb3 @xacrimon